Fix C Bitfield Macros [DEVINFRA-655]

[jungleraptor]

Description

This PR fixes a bug in the SET macros for bitfields.

The current SET macros will fail if the client attempts to set a non-zero bitfield value to a new value. I've added a new test to test_imu_bitfields in c/test/check_bitfield_macros.c that demonstrates the bug:

START_TEST(test_imu_bitfields) {
  // tow is a u32
  msg_imu_raw_t imu = {.tow = 100};
  // The time status are the two MSBs; 30-31
  SBP_IMU_RAW_TIME_STATUS_SET(
      imu.tow,
     SBP_IMU_RAW_TIME_STATUS_REFERENCE_EPOCH_IS_TIME_OF_SYSTEM_STARTUP /* = 1 */);
   // This works because the time status fields are 0
  fail_unless(imu.tow == (100 | 0x40000000));
  SBP_IMU_RAW_TIME_STATUS_SET(
    imu.tow,
    SBP_IMU_RAW_TIME_STATUS_REFERENCE_EPOCH_IS_UNKNOWN /* = 2 */);
  // The following fails; we expected 01 -> 10 (or 1 -> 2),
  // but instead we get 01 -> 11, because we OR the old fields with the new fields
  fail_unless(imu.tow == (100 | 0x80000000))
}

The change proposed clears the old values of the bitfields before setting the new value so that the expected result is correct.

@swift-nav/devinfra

API compatibility

The change was first proposed here (authored by @reimerix)

The contention was that the current behavior was correct for "flag" style bitfields, and the change might break clients relying on that behavior. The suggestion was to add a new macro with the desired behavior.

API compatibility plan

After further discussion and a review of our existing messages, we've concluded that the "flag" behavior was not supported to begin with.

Here is an example "flags" bitfield definition in our spec:

- PackedObsContent:
    short_desc: GNSS observations for a particular satellite signal
    fields:
        - flags:
            type: u8
            desc: >
              Measurement status flags.
            fields:
              - 7:
                  desc: RAIM exclusion
                  values:
                      - 0: No exclusion
                      - 1: Measurement was excluded by SPP RAIM, use with care
              - 4-6:
                  desc: Reserved
              - 3:
                  desc: Doppler valid
                  values:
                      - 0: Invalid doppler measurement
                      - 1: Valid doppler measurement
              - 2:
                  desc: Half-cycle ambiguity
                  values:
                      - 0: Half cycle phase ambiguity unresolved
                      - 1: Half cycle phase ambiguity resolved
              - 1:
                  desc: Carrier phase valid
                  values:
                      - 0: Invalid carrier phase measurement
                      - 1: Valid carrier phase measurement
              - 0:
                  desc: Pseudorange valid
                  values:
                      - 0: Invalid pseudorange measurement
                      - 1: Valid pseudorange measurement and coarse TOW decoded

Each "flag" is declared explicitly and corresponding GET, SET, MASK, and SHIFT macros are generated for each. The client is only able to SET a single bit at a time.

So for example clients cannot do the following:

u8 flags = 0x0F;

SET_PACKEDOBSCONTENT_RAIM_EXCLUSION(flags, 0x80);
fail_unless(flags == 0x8F); // fails, flags is still 0x0F

SET_SET_PACKEDOBSCONTENT_RAIM_EXCLUSION(flags, 1);
fail_unless(flags == 0x8F); // passes

This has been added to the test suite; test_packed_obs_bitfields to demonstrate this usage of the API is not supported.

The test passes before and after changing the SET implementation.

JIRA Reference

https://swift-nav.atlassian.net/browse/DEVINFRA-655

[silverjam]

FYI @reimerix -- mostly just your original change, but we've added some new tests and validated that this won't cause (unintended) backwards compatibility issues