rust: add bitfield types [DEVINFRA-615]

[notoriaga]

Something that's been missing from the rust client library is convenience functions around bitfields like in the c client. Pretty sure the generated code is not quite right, but looking to solicit some feedback on the approach generally.

For each message with a bitfield this generates

• msg.<field_name>() and msg.set_<field_name>(val) functions on the messages with the bitfield.
• if the spec includes a set of values for the bitfield a dataless enum is also created

So depending on whether or there is a set of values to work with, the generated code either produces getters/settings that work on integer types

impl MsgStatusReport {
    pub fn sbp_major_protocol_version_number(&self) -> u8 {
        get_bit_range!(self.sbp_version, u16, u8, 16, 8)
    }

    pub fn set_sbp_major_protocol_version_number(&mut self, sbp_major_protocol_version_number: u8) {
        set_bit_range!(
            &mut self.sbp_version,
            sbp_major_protocol_version_number,
            u16,
            u8,
            16,
            8
        );
    }

    pub fn sbp_minor_protocol_version_number(&self) -> u8 {
        get_bit_range!(self.sbp_version, u16, u8, 7, 0)
    }

    pub fn set_sbp_minor_protocol_version_number(&mut self, sbp_minor_protocol_version_number: u8) {
        set_bit_range!(
            &mut self.sbp_version,
            sbp_minor_protocol_version_number,
            u16,
            u8,
            7,
            0
        );
    }
}

Or ones that work with the newly defined enum

impl MsgBaselineEcef {
    pub fn fix_mode(&self) -> Option<MsgBaselineEcefFixMode> {
        match get_bit_range!(self.flags, u8, u8, 2, 0) {
            0 => Some(MsgBaselineEcefFixMode::Invalid),
            2 => Some(MsgBaselineEcefFixMode::DifferentialGnss),
            3 => Some(MsgBaselineEcefFixMode::FloatRtk),
            4 => Some(MsgBaselineEcefFixMode::FixedRtk),
            _ => None,
        }
    }

    pub fn set_fix_mode(&mut self, fix_mode: MsgBaselineEcefFixMode) {
        set_bit_range!(&mut self.flags, fix_mode, u8, u8, 2, 0);
    }
}

/// Fix mode
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum MsgBaselineEcefFixMode {
    /// Invalid
    Invalid = 0,

    /// Differential GNSS (DGNSS)
    DifferentialGnss = 2,

    /// Float RTK
    FloatRtk = 3,

    /// Fixed RTK
    FixedRtk = 4,
}

impl std::fmt::Display for MsgBaselineEcefFixMode {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            MsgBaselineEcefFixMode::Invalid => f.write_str("Invalid"),
            MsgBaselineEcefFixMode::DifferentialGnss => f.write_str("Differential GNSS (DGNSS)"),
            MsgBaselineEcefFixMode::FloatRtk => f.write_str("Float RTK"),
            MsgBaselineEcefFixMode::FixedRtk => f.write_str("Fixed RTK"),
        }
    }
}

With that here are some open questions I'm still thinking about

• It's kind of unfortunate that we have to return Option<MsgBaselineEcefFixMode>. I'm foreseeing this being unwrapped everywhere. Alternatives could be
  • panic instead of returning None. Convenient but less robust. I'm not sure if the spec allows this, but if for example a new fix mode is added, older clients getting a message with that fix mode would panic, whereas right now they'd just return None.
  • impl Default for these new types, and replace None with Default::default(). This makes sense for types that have a good candidate for a default value like Invalid, but not sure all the bitfields have a sensible default
  • Leaving it as is, the most convenient/robust way of using this api would probably be matching on the return value (including the option) so like if msg.fix_mode() == Some(MsgBaselineEcefFixMode::FloatRtk) { ... }, which isn't so bad I guess
• Currently the new types have derived impls of Debug, Clone, Copy, PartialEq, Eq and an impl of Display that is the desc field for the value. We can also impl PartialOrd, Ord, and Hash if those would be useful (not sure Ord makes sense semantically for all the different types). Also not sure if the Display is worth keeping. The descriptions aren't super descriptive, but maybe still helpful.
• Some of the varients of the enums start with a number, which isn't a valid rust identifier (even if made a raw identifier). So I just prepended a _ because that was the easiest thing to do
• Currently you can convert from the new enum types to an int by casting, but there is no way to go from int -> enum without matching. We could also provide something like a TryFrom impl from the integer types to the enums to make that more convenient.
• Adding a bunch of little methods to the messages reminds me that we should explore adding #[inline] throughout the library at some point. By default non-generic functions are not inlined across crates (my understanding of inlining in rust is basically 100% based on this post). We tend to use LTO for release builds which negates that issue but still might be worth taking a look at.

[notoriaga]

Some of the names are pretty long. We could have some manually curated set of replacements. Or we could add a name/identifier field to the part of the spec that defines bit field values instead of relying on the description

[notoriaga]

We could also drop the message name prefix part of the enum names if we moved each message into its own module

[silverjam]

We have this problem in the C bitfield support too, I suggested the same thing with having a curated or manually specified name-- the problem with that is needing to go through and create these. Maybe we can add this in later, and only do it for names that are particularly long and awkward, then fallback back to generation of the name if the "curated" name doesn't exist.

[silverjam]

• Leaving it as is, the most convenient/robust way of using this api would probably be matching on the return value (including the option) so like if msg.fix_mode() == Some(MsgBaselineEcefFixMode::FloatRtk) { ... }, which isn't so bad I guess

I think this behavior is fine given the issue with backwards compatibility with regards to panicking, we don't want to break old code because a new bitfield value was introduced. The semantics of bitfields varies too much to have a good default value.

Currently the new types have derived impls of Debug, Clone, Copy, PartialEq, Eq and an impl of Display that is the desc field for the value. We can also impl PartialOrd, Ord, and Hash if those would be useful (not sure Ord makes sense semantically for all the different types). Also not sure if the Display is worth keeping. The descriptions aren't super descriptive, but maybe still helpful.

The current set of derives seems fine, Hash might be useful though?

Currently you can convert from the new enum types to an int by casting, but there is no way to go from int -> enum without matching. We could also provide something like a TryFrom impl from the integer types to the enums to make that more convenient.

Implementing TryFrom sounds like a good idea to me.

[notoriaga]

Added Hash + TryFrom. Also tried moving each message into its own mod, definitely makes some of the names nicer but adds about 20k lines to the diff 😅

[jbangelo]

It's kind of unfortunate that we have to return Option. I'm foreseeing this being unwrapped everywhere.

Two other options spring to mind:

1. Return a Result<MsgBaselineEcefFixMode, UnknownBitfield<u8>>, this would allow reporting an unknown value to the calling code.
2. Don't use custom discriminants in the bitfield enums and include a Unknown(u8) variant in each one to wrap any unknown values.

Both would allow conserving and reporting any unknown values from parsed messages. I don't think the programs would be able to do much with these values, but it would be useful information to include in an error log message to indicate the version of libsbp might need to be updated. The first option kind of makes the unwrapping problem you foresee worse.

I think I prefer the second one since it doesn't treat new/unknown bitfield values as an error case. We could still provide a impl From<MsgBaselineEcefFixMode> for u8 for the bitfields and it could conserve unknown values which we are throwing out with the current approach. I think I'd keep the impl TryFrom<u8> for MsgBaselineEcefFixMode as fallible, if the unknown value is too large to fit into the bit field we'd probably want to signal a failure instead of implicitly masking off the unsupported bits.

[notoriaga]

I've been going thru icbins (which does a like of bitfield stuff) and seeing what it looks like with these changes. A pretty common operation I've noticed is checking that a bitfield value is not equal to one of it's variants (typically an Invalid or Unknown state). With the current setup that looks like

if msg.fix_mode() != Some(FixMode::Invalid) {
   ...
}

But if the unknown type is inside the enum you'd have to do something like

if !matches!(msg.fix_mode(), FixMode::Invalid | FixMode::UnknownBitfield(_)) {
   ...
}

Not a big problem, just an observation that it's easier to inspect one variant of a enum wrapped in a type, than it is to check two enum variants.

Additionally, in practice it's kind of nice to have the Unknown state as a separate type because you have more type-level info

if let Some(fix_mode) = msg.fix_mode() {
   // known by the compiler we have a valid fix mode
}

vs.

let fix_mode =  msg.fix_mode();
if !matches!(fix_mode, FixMode::UnknownBitfield(_)) {
   // only known by the programmer
}

That being said, I do really like the idea of preserving the unknown value, so maybe option 1 is a good choice? I think we could even simplify it a bit and do Result<FixMode, u8>. This would also hint that an unknown value is not necessarily an error. There is some precedence on not using a type that actually impls Error in Result, OnceCell::set comes to mind.

[notoriaga]

Now that I think about it, maybe the TryFrom<Sbp> impls for each message should have type Error = Sbp/have TryFromSbpError store the message.

[jbangelo]

Your justification for not including an unknown variant in the enums makes sense. I think it would still be good to include a documentation comment for the getter functions explaining that the "error" variant of the Result contains an unknown flag value. Is it reasonable/easy to include doc comments in the generated code?

[notoriaga]

Your justification for not including an unknown variant in the enums makes sense. I think it would still be good to include a documentation comment for the getter functions explaining that the "error" variant of the Result contains an unknown flag value. Is it reasonable/easy to include doc comments in the generated code?

Yeah we could do something like

/// Gets the [FixMode][FixMode] stored in the `flags` field.
///
/// Returns `Ok` if `flags` holds a known `FixMode`. Otherwise the value of the bitfield is returned
/// in the `Err` variant. This may be because of a malformed message, or because new variants of `FixMode`
/// were added (in which case you should update this library).
pub fn fix_mode(&self) -> Result<FixMode, u8> {
    get_bit_range!(self.flags, u8, u8, 2, 0).try_into()
}

Probably a better/shorter way to word this, but in general something like that would be easy to set up

[notoriaga]

Another quirk I noticed in the spec is that few message have single-bit fields that don't have values defined

MSG_NETWORK_STATE_RESP bit 15 of flags (iff_multicast_supports_multicast)
MSG_NETWORK_STATE_RESP bit 14 of flags (iff_link2_per_link_layer_defined_bit)
MSG_NETWORK_STATE_RESP bit 13 of flags (iff_link1_per_link_layer_defined_bit)
MSG_NETWORK_STATE_RESP bit 12 of flags (iff_link0_per_link_layer_defined_bit)
MSG_NETWORK_STATE_RESP bit 11 of flags (iff_simplex_cant_hear_own_transmissions)
MSG_NETWORK_STATE_RESP bit 10 of flags (iff_oactive_transmission_in_progress)
MSG_NETWORK_STATE_RESP bit 9 of flags (iff_allmulti_receive_all_multicast_packets)
MSG_NETWORK_STATE_RESP bit 8 of flags (iff_promisc_receive_all_packets)
MSG_NETWORK_STATE_RESP bit 7 of flags (iff_noarp_no_address_resolution_protocol)
MSG_NETWORK_STATE_RESP bit 6 of flags (iff_running_resources_allocated)
MSG_NETWORK_STATE_RESP bit 5 of flags (iff_notrailers_avoid_use_of_trailers)
MSG_NETWORK_STATE_RESP bit 4 of flags (iff_pointopoint_interface_is_pointtopoint_link)
MSG_NETWORK_STATE_RESP bit 3 of flags (iff_loopback_is_a_loopback_net)
MSG_NETWORK_STATE_RESP bit 2 of flags (iff_debug_broadcast_address_valid)
MSG_NETWORK_STATE_RESP bit 1 of flags (iff_broadcast_broadcast_address_valid)
MSG_NETWORK_STATE_RESP bit 0 of flags (iff_up_interface_is_up)
MSG_DOPS bit 7 of flags (raim_repair_flag)
MSG_PROTECTION_LEVEL bit 21 of flags (velocity_valid)
MSG_PROTECTION_LEVEL bit 22 of flags (attitude_valid)
MSG_PROTECTION_LEVEL bit 23 of flags (safe_state_hpl)
MSG_PROTECTION_LEVEL bit 24 of flags (safe_state_vpl)
MSG_PROTECTION_LEVEL bit 25 of flags (safe_state_atpl)
MSG_PROTECTION_LEVEL bit 26 of flags (safe_state_ctpl)
MSG_PROTECTION_LEVEL bit 27 of flags (safe_state_hvpl)
MSG_PROTECTION_LEVEL bit 28 of flags (safe_state_vvpl)
MSG_PROTECTION_LEVEL bit 29 of flags (safe_state_hopl)
MSG_PROTECTION_LEVEL bit 30 of flags (safe_state_popl)
MSG_PROTECTION_LEVEL bit 31 of flags (safe_state_ropl)

Right now this will generate

/// Sets the `safe_state_hopl` bitrange of `flags`.
pub fn set_safe_state_hopl(&mut self, safe_state_hopl: u8) {
    set_bit_range!(&mut self.flags, safe_state_hopl, u32, u8, 29, 29);
}

For single-bit fields we could switch it to accept/return bool. But I think the better solution would be to add the missing values so that we can generate types for these bitfields so people don't have to think through what true/false means in each case. Will probably need some input on what each of these fields mean though to do this properly.

[notoriaga]

Okay so an example of each case -

If the spec has values generate a type and use that. Unknown values are smuggled in the Err

/// Gets the [FixMode][self::FixMode] stored in the `flags` bitfield.
///
/// Returns `Ok` if the bitrange contains a known `FixMode` variant.
/// Otherwise the value of the bitrange is returned as an `Err(u8)`. This may be because of a malformed message,
/// or because new variants of `FixMode` were added.
pub fn fix_mode(&self) -> Result<FixMode, u8> {
    get_bit_range!(self.flags, u8, u8, 2, 0).try_into()
}

/// Sets the bitrange corresponding to the [FixMode][FixMode] of the `flags` bitfield.
pub fn set_fix_mode(&mut self, fix_mode: FixMode) {
    set_bit_range!(&mut self.flags, fix_mode, u8, u8, 2, 0);
}

/// Fix mode
#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)]
pub enum FixMode {
    /// Invalid
    Invalid = 0,

    /// Single Point Position (SPP)
    SinglePointPosition = 1,

    /// Differential GNSS (DGNSS)
    DifferentialGnss = 2,

    /// Float RTK
    FloatRtk = 3,

    /// Fixed RTK
    FixedRtk = 4,

    /// Dead Reckoning
    DeadReckoning = 5,

    /// SBAS Position
    SbasPosition = 6,
}

impl std::fmt::Display for FixMode {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        match self {
            FixMode::Invalid => f.write_str("Invalid"),
            FixMode::SinglePointPosition => f.write_str("Single Point Position (SPP)"),
            FixMode::DifferentialGnss => f.write_str("Differential GNSS (DGNSS)"),
            FixMode::FloatRtk => f.write_str("Float RTK"),
            FixMode::FixedRtk => f.write_str("Fixed RTK"),
            FixMode::DeadReckoning => f.write_str("Dead Reckoning"),
            FixMode::SbasPosition => f.write_str("SBAS Position"),
        }
    }
}

impl TryFrom<u8> for FixMode {
    type Error = u8;
    fn try_from(i: u8) -> Result<Self, Self::Error> {
        match i {
            0 => Ok(FixMode::Invalid),
            1 => Ok(FixMode::SinglePointPosition),
            2 => Ok(FixMode::DifferentialGnss),
            3 => Ok(FixMode::FloatRtk),
            4 => Ok(FixMode::FixedRtk),
            5 => Ok(FixMode::DeadReckoning),
            6 => Ok(FixMode::SbasPosition),
            i => Err(i),
        }
    }
}

If the spec has no values and the bitfield's length > 1

/// Gets the `time_since_reference_epoch_in_milliseconds` stored in `tow`.
pub fn time_since_reference_epoch_in_milliseconds(&self) -> u32 {
    get_bit_range!(self.tow, u32, u32, 29, 0)
}

/// Sets the `time_since_reference_epoch_in_milliseconds` bitrange of `tow`.
pub fn set_time_since_reference_epoch_in_milliseconds(
    &mut self,
    time_since_reference_epoch_in_milliseconds: u32,
) {
    set_bit_range!(
        &mut self.tow,
        time_since_reference_epoch_in_milliseconds,
        u32,
        u32,
        29,
        0
    );
}

Should the setter here error is the value is greater than the bitrange would allow?

Finally if the bitfield has length one we use bools

/// Gets the `velocity_valid` flag.
pub fn velocity_valid(&self) -> bool {
    ((self.flags >> 21) & 1) == 1
}

/// Sets the `velocity_valid` flag.
pub fn set_velocity_valid(&mut self, velocity_valid: bool) {
    self.flags ^= (!(velocity_valid as u32)) & (1 << 21)
}

still need to make sure everything works correctly bits make my head hurt

[silverjam]

Let's add an example for each case the ship it?

@jbangelo Any more feedback?

[notoriaga]

Added some examples to the docs

[notoriaga]

Small error in the spec and just ran the c generator because I think it's the only other language using this feature

[silverjam]

Looks like rust generator needs to run again but otherwise lgtm