[CONSULT-469] - introduction of NMEA message macro #386

gvaradarajan · 2025-01-22T17:38:47Z

This is the first PR actually introducing the procedural macro. As explained in the previous commit, it is recommended that one uses cargo expand in the micro-rdk-nmea directory to view the code that is generated by the macro used in the example PGN structs.

This PR includes

parsing logic for the message header
logic for parsing a message struct used by the procedural macro itself
procedural code generation for messages using number and lookup field types

The next PR will include

procedural code generation support for array and fieldset fields (also understood as "repeated fields")

npmenard

Done with first pass. I like what i am seeing, the code generation is very mechanical and doesn't have too much custom logic.

I see that you generate *_raw function to access fields, was it impossible to reference the field immediately in the code gen? Do you foresee special logic in these functions?Do you mind adding a couple more examples showing up how to use field annotations?
I think conversion is hard to read at the moment, however I recall this is mostly how goNMEA does it so I would leave it as is but add a tickets for potential further work

We should also avoid returning errors in to_readings as much as possible

npmenard · 2025-01-24T14:22:11Z

micro-rdk-nmea-macros/src/composition.rs

+            #max_token
+            let result = match result {
+                x if x == max => { return Err(#error_ident::FieldNotPresent(#name_as_string_ident.to_string())); },
+                x if x == (max - 1) => { return Err(#error_ident::FieldError(#name_as_string_ident.to_string())); },


This is interesting, if the field has an invalid value the err will bubble out and to_readings won't return an hashmap. I think we should return a value here, ideally it should be a string like "invalid" or "absent". This may imply that this function will return a value::Kind so that to_reading don't have extra logic to wrap the error in a value::Kind::String.
How is this case handle in gonmea? if this is how it's done then let's leave it as is

Digging through the GoNMEA logic, it appears that the library does not return a message instance if any number field produces an error value. However, what is missing is that number fields less than 4 bits in size use an alternate logic. I'll update with this change

npmenard · 2025-01-24T14:30:26Z

micro-rdk-nmea-macros/src/composition.rs

+                });
+                statements
+                    .struct_initialization
+                    .push(quote! { source_id: source_id.unwrap(), });


i think we should remove the unwrap either make it mandatory or default to a sensible value

it shouldn't be an option in the first place, will fix

npmenard · 2025-01-24T15:28:16Z

micro-rdk-nmea-macros/src/utils.rs

+    quote! {#crate_ident::google::protobuf}
+}
+
+pub(crate) fn determine_supported_numeric(field_type: &Type) -> bool {


is_supported_integer_type

npmenard · 2025-01-24T15:33:24Z

micro-rdk-nmea-macros/src/lib.rs

+use proc_macro2::Span;
+use syn::DeriveInput;
+
+fn get_statements(input: &DeriveInput) -> Result<PgnComposition, TokenStream> {


I was wondering why you have function that do not take self. It is impossible to have a Struct that holds all statements, fields etc... that can mutate self?

I don't remember why, I'll try changing it to an instance method and see if I run into any problems

acmorrow

Also done with a first pass through

acmorrow · 2025-01-23T21:10:40Z

micro-rdk-nmea/Cargo.toml

+micro-rdk = { path = "../micro-rdk", features = ["native"] }
+micro-rdk-nmea-macros = { path = "../micro-rdk-nmea-macros" }


Previously micro-rdk came via a workspace = true, which I prefer here. I'd expect the same would work for micro-rdk-nmea-macros if you introduced it in the root Cargo.toml.

micro-rdk-nmea-macros/src/attributes.rs

acmorrow · 2025-01-23T21:13:36Z

micro-rdk-nmea-macros/src/attributes.rs

+// `MacroAttributes` represents the important information derived from the macro attributes
+// attached to the field of a struct using the PgnMessageDerive macro.
+//
+// bits -> size in bits of the field in the NMEA message byte slice


nit: maybe : instead of ->. Also, maybe, `bits`, etc. since you did it for `MacroAttributes` above?

Also, should this be a /// doc comment?

acmorrow · 2025-01-23T21:23:41Z

micro-rdk-nmea/src/lib.rs

+        let mut data = Vec::<u8>::new();
+        let res = general_purpose::STANDARD.decode_vec(water_depth_str, &mut data);
+        assert!(res.is_ok());
+        let message = WaterDepth::from_bytes(data[33..].to_vec(), Some(13));


Why 33 bytes in? What does Some(13) mean here?

The first part is metadata, I'll put a comment in

micro-rdk-nmea/src/lib.rs

acmorrow · 2025-01-23T21:47:40Z

micro-rdk-nmea/src/parse_helpers/parsers.rs

+    pub fn src(&self) -> u16 {
+        self.src
+    }
+
+    pub fn dst(&self) -> u16 {
+        self.dst
+    }
+
+    pub fn priority(&self) -> u16 {
+        self.priority
+    }
+
+    pub fn timestamp(&self) -> Timestamp {
+        self.timestamp.clone()
+    }


Are these trivial accessors needed? Is there some reason that if you had a mut NmeaMessageMetadata that it should be impossible to alter the values?

In principle, I think data coming from a different source should be left alone as much as possible? I think you wouldn't want to alter the data unless it was absolutely necessary for conversion purposes

acmorrow · 2025-01-23T21:49:26Z

micro-rdk-nmea/src/parse_helpers/parsers.rs

@@ -213,16 +213,82 @@ where
    }
 }

+// The first 32 bytes of the unparsed message stores metadata appearing in the header


I'm having difficulty lining up this comment with what I found at the link, which seems to talk about 39 bits?

I should look into this further and not have referenced the link. When examining the raw data that we are currently pushing up (and also looking at the logic in the GoNMEA library), it was clear that the format of the message after processing CAN frames was a 32-byte header followed by the message contents. I admit to a poor understanding of what happens in between receipt of the data from a CAN bus and seeing the data in this format

acmorrow · 2025-01-23T21:51:13Z

micro-rdk-nmea/src/parse_helpers/parsers.rs

+// this data from the CAN frame). For an explanation of the fields in this header, see
+// the section labeled "ISO-11783 and NMEA2000 header" at https://canboat.github.io/canboat/canboat.html
+pub struct NmeaMessageMetadata {
+    timestamp: Timestamp,


Why as a proto Timestamp, rather than some non-proto type, something from std, etc.?

Honestly, it was laziness. It was already in seconds and milliseconds and it was easier to skip to serializing it for the API immediately

Ok, I'm going to change it to something not proto

acmorrow · 2025-01-23T22:28:03Z

micro-rdk-nmea-macros/src/utils.rs

+    RadPerSecToDegPerSec,
+}
+
+impl TryFrom<String> for UnitConversion {


It bothers me a lot that impl<T> TryFrom<T> for UnitConversion where T: AsRef<str> { cannot work, per https://users.rust-lang.org/t/confusion-interpreting-error-when-using-tryfrom-with-asref/83672.

yeah me too

acmorrow · 2025-01-23T22:31:00Z

micro-rdk-nmea-macros/src/utils.rs

+
+impl TryFrom<String> for UnitConversion {
+    type Error = TokenStream;
+    fn try_from(value: String) -> Result<Self, Self::Error> {


You don't actually need to consume the string here, right? Maybe this should be &str?

yeah, you're right, that's better

gvaradarajan · 2025-01-24T18:57:26Z

Done with first pass. I like what i am seeing, the code generation is very mechanical and doesn't have too much custom logic.

I see that you generate *_raw function to access fields, was it impossible to reference the field immediately in the code gen? Do you foresee special logic in these functions?Do you mind adding a couple more examples showing up how to use field annotations? I think conversion is hard to read at the moment, however I recall this is mostly how goNMEA does it so I would leave it as is but add a tickets for potential further work

We should also avoid returning errors in to_readings as much as possible

@npmenard Regarding the raw accessors: they are not used by the auto-generated code, I included them for ease in debugging potential data issues within a single specific message instance. Should I remove them?

acmorrow

LGTM

acmorrow · 2025-01-24T20:13:37Z

micro-rdk-nmea-macros/src/attributes.rs

+
+#[derive(Debug)]
+pub(crate) struct MacroAttributes {
+    pub(crate) bits: Option<usize>,


bits shouldn't be optional

acmorrow · 2025-01-24T20:14:40Z

micro-rdk-nmea-macros/src/attributes.rs

+        Type::Path(type_path) if type_path.path.is_ident("i64") => 64,
+        Type::Path(type_path) if type_path.path.is_ident("u64") => 64,


Sort these in

acmorrow · 2025-01-24T20:23:12Z

micro-rdk-nmea-macros/src/composition.rs

+            x if x > 4 => quote! {
+                #max_token
+                let result = match result {
+                    x if x == max => { return Err(#error_ident::FieldNotPresent(#name_as_string_ident.to_string())); },
+                    x => {
+                        (x as f64) * #scale_token
+                    }
+                };
+            },
+            x if x >= 4 => quote! {


These don't look like they are exclusive

[CONSULT-469] - introduction of NMEA message macro

759ec36

gvaradarajan requested a review from a team as a code owner January 22, 2025 17:38

gvaradarajan requested review from npmenard and acmorrow January 22, 2025 17:38

gvaradarajan and others added 3 commits January 22, 2025 13:37

missed a change

96c254c

ok, last missed change

e9453aa

Merge branch 'main' into nmea-parsing-pgn-macro

7989da0

npmenard reviewed Jan 24, 2025

View reviewed changes

acmorrow reviewed Jan 24, 2025

View reviewed changes

address review comments

a925ddc

gvaradarajan requested review from npmenard and acmorrow January 24, 2025 19:20

acmorrow approved these changes Jan 24, 2025

View reviewed changes

fix + small adjustments

6d2120d

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CONSULT-469] - introduction of NMEA message macro #386

[CONSULT-469] - introduction of NMEA message macro #386

gvaradarajan commented Jan 22, 2025

npmenard left a comment

npmenard Jan 24, 2025

gvaradarajan Jan 24, 2025

npmenard Jan 24, 2025

gvaradarajan Jan 24, 2025

npmenard Jan 24, 2025

npmenard Jan 24, 2025

gvaradarajan Jan 24, 2025

acmorrow left a comment

acmorrow Jan 23, 2025

acmorrow Jan 23, 2025

acmorrow Jan 23, 2025

gvaradarajan Jan 24, 2025

acmorrow Jan 23, 2025

gvaradarajan Jan 24, 2025 •

edited

Loading

acmorrow Jan 23, 2025

gvaradarajan Jan 24, 2025

acmorrow Jan 23, 2025

gvaradarajan Jan 24, 2025

gvaradarajan Jan 24, 2025

acmorrow Jan 23, 2025

gvaradarajan Jan 24, 2025

acmorrow Jan 23, 2025

gvaradarajan Jan 24, 2025

gvaradarajan commented Jan 24, 2025

acmorrow left a comment

acmorrow Jan 24, 2025

acmorrow Jan 24, 2025

acmorrow Jan 24, 2025

		micro-rdk = { path = "../micro-rdk", features = ["native"] }
		micro-rdk-nmea-macros = { path = "../micro-rdk-nmea-macros" }

		Type::Path(type_path) if type_path.path.is_ident("i64") => 64,
		Type::Path(type_path) if type_path.path.is_ident("u64") => 64,

[CONSULT-469] - introduction of NMEA message macro #386

Are you sure you want to change the base?

[CONSULT-469] - introduction of NMEA message macro #386

Conversation

gvaradarajan commented Jan 22, 2025

npmenard 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

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

acmorrow 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvaradarajan Jan 24, 2025 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gvaradarajan commented Jan 24, 2025

acmorrow 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

gvaradarajan Jan 24, 2025 •

edited

Loading