`no_std` #41

patrickoppel · 2022-07-16T03:07:56Z

Try at an implementation replacing std::Vec with heapless::Vec as mentioned in #10

elpiel

@Dushistov @hargoniX would any of you be able to take a look at this PR too?

src/sentences/gsa.rs

Cargo.toml

src/sentences/gsa.rs

src/sentences/gsv.rs

elpiel · 2022-07-16T13:32:06Z

Some of the changes actually broke the parse, I took the code and fixed almost all the issues.
only the test_gsv_real_data tests fails right now

fix bugs with `heapless` rewrite & cleanup

elpiel · 2022-07-16T14:04:20Z

I made a few improvements on the functional_tests .
the failing test has the same GPS satellites twice and I'm not sure which data should be in the expected parsed data.

functional_tests - improve readability and use vec & no_std attribute

patrickoppel · 2022-07-17T06:26:49Z

Having the same satellite in the satellites_scan twice should be an error, currently the implementation in satellites() returns the first mention of it in the GSV data. In the real world this should never happen with NMEA data.
So I think there's two possibilities:

leave satellites() as is and change the test to reflect the first mention (this)
implement a check and error in merge_gsv_data() and write another test to test for the error

elpiel · 2022-07-17T08:25:45Z

Ok, I found the problem @patrickoppel. You forgot to rev() when you were refactoring the code.
The NMEA sentences can contain multiple satellites twice, this is because such a log can contain multiple received messages.

- revert functional_tests data

patrickoppel · 2022-07-17T08:31:49Z

If it can contain multiple satellites twice the total number of satellites in view can be a lot higher than what we set now

elpiel · 2022-07-17T08:40:11Z

This should actually be ready now:
patrickoppel#3

Fixed the test data and the missing rev(), removed alloc, some docs fixes and we're now officially no_std 🎉 (I've added the category to Cargo.toml as well)

No std test fix & last `no_std` preparations

codecov · 2022-07-17T10:27:17Z

Codecov Report

Merging #41 (b3b4370) into master (80b8057) will increase coverage by 1.08%.
The diff coverage is 88.60%.

@@            Coverage Diff             @@
##           master      #41      +/-   ##
==========================================
+ Coverage   87.12%   88.21%   +1.08%     
==========================================
  Files          15       15              
  Lines        1212     1222      +10     
==========================================
+ Hits         1056     1078      +22     
+ Misses        156      144      -12

Impacted Files	Coverage Δ
src/lib.rs	`81.90% <53.33%> (+3.55%)`	⬆️
src/sentences/gsa.rs	`91.13% <91.30%> (-0.80%)`	⬇️
src/sentences/bwc.rs	`96.49% <100.00%> (ø)`
src/sentences/gsv.rs	`95.34% <100.00%> (+0.05%)`	⬆️
tests/file_log_parser/mod.rs	`90.00% <100.00%> (ø)`
tests/functional_tests.rs	`98.94% <100.00%> (+0.02%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 80b8057...b3b4370. Read the comment docs.

Dushistov · 2022-07-17T10:15:12Z

src/lib.rs

@@ -72,7 +72,7 @@ pub struct Nmea {
    pub pdop: Option<f32>,
    /// Geoid separation in meters
    pub geoid_separation: Option<f32>,
-    pub fix_satellites_prns: Option<Vec<u32>>,
+    pub fix_satellites_prns: Option<Vec<u32, 12>>,


I suppose it should be 58, but it can wait until we can teach this crate to merge GSA messages.

Ok, Can you also provide some info on this merging?

Dushistov · 2022-07-17T10:20:40Z

src/lib.rs

-    pub fn satellites(&self) -> Vec<Satellite> {
-        let mut ret = Vec::with_capacity(20);
+    /// Returns used satellites
+    pub fn satellites(&self) -> Vec<Satellite, 58> {


It would be nice if 58 become constant with comment why exactly 58.
And it would be nice to replace 15 with constant time expression that calculate it from 58 in SatsPack.

I don't know what the limits are for these 2 values tbh and I feel more comfortable gradually improving them.
I feel that the code might get more complex especially with the public API and those constant values.

I think we can improve the API a lot and see how we can do the merging of the messages you mentioed.

Dushistov · 2022-07-17T10:21:15Z

src/lib.rs

        let sat_key = |sat: &Satellite| (sat.gnss_type() as u8, sat.prn());
        for sns in &self.satellites_scan {
-            for sat_pack in sns.data.iter().rev() {
-                for sat in sat_pack.iter().flatten() {
+            // for sat_pack in sns.data.iter().rev() {


Why not remove dead code?

Dushistov · 2022-07-17T10:23:24Z

src/sentences/gsa.rs

+    O: core::fmt::Debug,
+{
+    move |mut i: I| {
+        // let mut acc = crate::lib::std::vec::Vec::with_capacity(4);


Dead but this is the original nom code. I think it would be good to leave it as it is.

Dushistov · 2022-07-17T10:38:00Z

If it can contain multiple satellites twice the total number of satellites in view can be a lot higher than what we set now

I wonder to see logs of GPS receiver that can work with L1 and L2(C) at the same time.
Would they give the same PRN in one GSA message?

elpiel

I wonder to see logs of GPS receiver that can work with L1 and L2(C) at the same time.
Would they give the same PRN in one GSA message?

I don't know actually. Maybe if a problem arises we can fix it in the library or if we can find this information somewhere?

elpiel · 2022-07-17T11:06:22Z

src/sentences/gsa.rs

+    O: core::fmt::Debug,
+{
+    move |mut i: I| {
+        // let mut acc = crate::lib::std::vec::Vec::with_capacity(4);


Dead but this is the original nom code. I think it would be good to leave it as it is.

elpiel · 2022-07-17T11:07:21Z

src/lib.rs

@@ -72,7 +72,7 @@ pub struct Nmea {
    pub pdop: Option<f32>,
    /// Geoid separation in meters
    pub geoid_separation: Option<f32>,
-    pub fix_satellites_prns: Option<Vec<u32>>,
+    pub fix_satellites_prns: Option<Vec<u32, 12>>,


Ok, Can you also provide some info on this merging?

elpiel · 2022-07-17T11:25:19Z

src/lib.rs

-    pub fn satellites(&self) -> Vec<Satellite> {
-        let mut ret = Vec::with_capacity(20);
+    /// Returns used satellites
+    pub fn satellites(&self) -> Vec<Satellite, 58> {


I don't know what the limits are for these 2 values tbh and I feel more comfortable gradually improving them.
I feel that the code might get more complex especially with the public API and those constant values.

I think we can improve the API a lot and see how we can do the merging of the messages you mentioed.

patrickoppel · 2022-07-17T12:48:07Z

I wouldn't expect them to. If I remember it right, dual band allows the receiver to increase the accuracy of the satellites' positions. So it would still use the same number of satellites and return them in the GSA

elpiel · 2022-08-10T10:25:37Z

I think this is good enough for now. If any problem arise we will fix them in subsequent PR.

patrickoppel and others added 2 commits July 16, 2022 12:54

remove std

c883bec

Update Cargo.toml

e0d14aa

elpiel reviewed Jul 16, 2022

View reviewed changes

This was linked to issues Jul 16, 2022

Try to avoid alloc #13

Closed

no_std #10

Closed

fix bugs with heapless rewrite & cleanup

45e7b45

elpiel mentioned this pull request Jul 16, 2022

fix bugs with heapless rewrite & cleanup patrickoppel/nmea#1

Merged

patrickoppel and others added 2 commits July 16, 2022 23:45

Merge pull request #1 from AeroRust/no_std-test-fixes

3c46eb0

fix bugs with `heapless` rewrite & cleanup

functional_tests - improve readability and use vec

776be89

elpiel mentioned this pull request Jul 16, 2022

functional_tests - improve readability and use vec & no_std attribute patrickoppel/nmea#2

Merged

elpiel and others added 3 commits July 16, 2022 16:09

file_log_parse - fix test

d6eb8ed

lib - add cfg attribute for std

533f16e

Merge pull request #2 from AeroRust/no_std-test-fixes

3e904ef

functional_tests - improve readability and use vec & no_std attribute

change test_gsv_real_data, set satellites to 58

dc688fd

NMEA::satellites - add missing rev()

85b7ff3

- revert functional_tests data

elpiel added 2 commits July 17, 2022 10:33

Merge branch 'master' into no_std-test-fix

ef42604

no_std & no_alloc, final docs & rustfmt

8819e59

sentences - gsa - clean up test & fix clippy warning

4717a09

elpiel changed the title ~~No_std~~ no_std Jul 17, 2022

Merge pull request #3 from AeroRust/no_std-test-fix

b3b4370

No std test fix & last `no_std` preparations

Dushistov reviewed Jul 17, 2022

View reviewed changes

elpiel reviewed Jul 17, 2022

View reviewed changes

elpiel requested a review from Dushistov August 5, 2022 12:40

elpiel force-pushed the master branch from 33825f7 to 9241043 Compare August 10, 2022 08:48

elpiel merged commit 8a90195 into AeroRust:main Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`no_std` #41

`no_std` #41

patrickoppel commented Jul 16, 2022

elpiel left a comment •

edited

Loading

elpiel commented Jul 16, 2022

elpiel commented Jul 16, 2022 •

edited

Loading

patrickoppel commented Jul 17, 2022 •

edited

Loading

elpiel commented Jul 17, 2022

patrickoppel commented Jul 17, 2022

elpiel commented Jul 17, 2022

codecov bot commented Jul 17, 2022

Dushistov Jul 17, 2022

elpiel Jul 17, 2022

Dushistov Jul 17, 2022

elpiel Jul 17, 2022

Dushistov Jul 17, 2022

Dushistov Jul 17, 2022

elpiel Jul 17, 2022

Dushistov commented Jul 17, 2022

elpiel left a comment

elpiel Jul 17, 2022

elpiel Jul 17, 2022

elpiel Jul 17, 2022

patrickoppel commented Jul 17, 2022

elpiel commented Aug 10, 2022

no_std #41

no_std #41

Conversation

patrickoppel commented Jul 16, 2022

elpiel left a comment • edited Loading

Choose a reason for hiding this comment

elpiel commented Jul 16, 2022

elpiel commented Jul 16, 2022 • edited Loading

patrickoppel commented Jul 17, 2022 • edited Loading

elpiel commented Jul 17, 2022

patrickoppel commented Jul 17, 2022

elpiel commented Jul 17, 2022

codecov bot commented Jul 17, 2022

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dushistov commented Jul 17, 2022

elpiel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickoppel commented Jul 17, 2022

elpiel commented Aug 10, 2022

`no_std` #41

`no_std` #41

elpiel left a comment •

edited

Loading

elpiel commented Jul 16, 2022 •

edited

Loading

patrickoppel commented Jul 17, 2022 •

edited

Loading