Fix bitfield C macros #1099
Fix bitfield C macros #1099
Conversation
| @@ -316,7 +316,7 @@ def create_bitfield_macros(field, msg): | |||
| bitrange = (item.get("range")).split(":") | |||
| start_bit = int(bitrange[0]) | |||
| ret_list.append( | |||
| "#define {}_MASK ({})".format(base_string, hex((1 << nbits) - 1)) | |||
| "#define {}_MASK ({}u)".format(base_string, hex((1 << nbits) - 1)) | |||
There was a problem hiding this comment.
The changes in this file are the only ones that need to be reviewed - the rest was auto-generated.
It'd be nice if we could add this to the test generation framework. We recently added bitfield helpers to the rust client, so we already have two languages in which it'd be nice to have tests around this stuff. I also recently caught an error where the lsb/msb range was incorrect for a message in the spec. That'd be a bit more work than just adding a few unit tests here, @silverjam what do you think maybe track this on the dev infra board? |
There was a problem hiding this comment.
Please do not merge yet, since we've already published this behavior we need to think carefully about how this will affect existing users-- it looks like the existing macro was acting more like a |= -- which makes sense for "flag" style bitfields, but does not make sense for "value" style bitfields.
Some alternatives we should consider that are backwards compatible (or at least provide a clear migration path):
- creating a new macro
SBP_<msg>_<bitfield>_RESET-- either clears the value, or takes a value to reset the bitfield to - rev'ing the major version of the SBP library, and removing these macros, replacing with
SBP_<msg>_<bitfield>_SET_FLAGandSBP_<msg>_<bitfield>_SET_VALUE.
|
Created https://swift-nav.atlassian.net/browse/DEVINFRA-655 to track (for next sprint) |
|
Closing since this will be covered in a different PR by @isaactorz for https://swift-nav.atlassian.net/browse/DEVINFRA-655 |
This PR fixes up the bitfield setting macros. The previous definition resulted in unexpected results because the bitfield was not reset before bitwise-or-ing the new value.