Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CONSULT-469] - macro support for array and repeating fields in NMEA #392

Merged
merged 7 commits into from
Jan 30, 2025

Conversation

gvaradarajan
Copy link
Member

Also contains the following fixes

  • adjustment for number field parsing (it turns out I was somewhat mistaken on the format)
  • incorrect advancement of the cursor for when a field is annotated with the offset property

@gvaradarajan gvaradarajan requested a review from a team as a code owner January 27, 2025 21:34
@gvaradarajan gvaradarajan requested a review from acmorrow January 27, 2025 21:34
Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, I'm fairly happy with this. I like the way the field set declarations work. A few questions. It might be worth sitting down together for 30 to talk through a few things, mostly for my understanding.

@@ -330,11 +332,10 @@ mod tests {
let data_vec: Vec<u8> = vec![154, 6, 125, 179, 152, 113];
let mut cursor = DataCursor::new(data_vec);

// 154 is 10011010, reading the first 3 bits should yield 100 = 4
// 154 is 10011010, reading the last 3 bits should yield 010 = 4
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is stale

@@ -306,8 +308,8 @@ mod tests {
assert!(cursor.read(16).is_ok());
let res = reader.read_from_cursor(&mut cursor);
assert!(res.is_ok());
// 125 = 01111101, first four bits as byte => 00000111 = 7
assert_eq!(res.unwrap(), 7);
// 125 = 01111101, last four bits as byte => 00001101 = 7
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment is stale

micro-rdk-nmea-macros/src/composition.rs Show resolved Hide resolved
micro-rdk-nmea-macros/src/composition.rs Show resolved Hide resolved
@gvaradarajan gvaradarajan requested a review from acmorrow January 30, 2025 18:03
Copy link
Member

@acmorrow acmorrow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I get it. There is what looks one small bug which should be fixed before push, and a thing I want you to confirm back, but other than that it LGTM.

@@ -51,16 +57,23 @@ impl PgnComposition {

let macro_attrs = MacroAttributes::from_field(field)?;
if macro_attrs.offset != 0 {
let offset = macro_attrs.offset / 8;
let offset = macro_attrs.offset;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was the / 8 a bug? Or did the API of cursor.read change? Meaning of #offset?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it was a bug

}

pub fn read(&mut self, bit_size: usize) -> Result<Vec<u8>, NmeaParseError> {
if bit_size / 8 > self.data.len() {
let bits_to_read = bit_size + self.bit_offset;
if bits_to_read / 8 > self.data.len() {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this won't work, because if you had one byte left, but bits_to_read was 15, this check would pass even though you only had 8 bits of data left.

Maybe if bits_to_read > self.data.len() * 8 is clearer?

let mask: u8 = 255 >> (bit_size % 8);
if let Some(first_byte) = self.data.get_mut(0) {
*first_byte |= mask;
res.extend(self.data.drain(..(bits_to_read / 8)));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, just confirming, that here we are intentionally "under" reading. If bits_to_read is 15, we actually only want to read one byte permanently, then manually push a munged value we compute out of the byte we didn't permanently read, so that we can set the cursors bit_offset and leave it pointing to the partially read byte. Do I have that right?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that is correct

@gvaradarajan gvaradarajan merged commit 895b960 into viamrobotics:main Jan 30, 2025
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants