Attention is currently required from: neels, pespin, dexter.

View Change

1 comment:

• File include/osmocom/gsm/protocol/gsm_04_08.h:

  • Patch Set #2, Line 627: return ((uint8_t *)cfg)[1];

    gsm48_multi_rate_conf has two uint8_t members, AFAICT adding a union would require adding another ex […]

    You don't really need to name the union itself, it can be anonymous (just like structures). The problem is that bit-fields are treated as individual fields in a union, so doing what Pau suggested:

    ```
    /* I removed the LE variant for the sake of readability
     * see https://people.osmocom.org/fixeria/union_bfield_test.c */
    struct gsm48_multi_rate_conf {
      uint8_t ver:3, nscb:1, icmi:1, spare:1, smod:2;
      union {
        uint8_t m12_2:1, m10_2:1, m7_95:1, m7_40:1,
                m6_70:1, m5_90:1, m5_15:1, m4_75:1;
        uint8_t mode;
      };
    } __attribute__((packed));
    ```

    will result in the following behavior:

    ```
    struct gsm48_multi_rate_conf mrc = { 0 };
    // mrc.mode = 0x00
    // m4_75=0, m5_15=0, m5_90=0, m6_70=0, m7_40=0, m7_95=0, m10_2=0, m12_2=0

    mrc.m4_75 = 1;
    // mrc.mode = 0x01
    // m4_75=1, m5_15=1, m5_90=1, m6_70=1, m7_40=1, m7_95=1, m10_2=1, m12_2=1

    mrc.m10_2 = 0;
    // mrc.mode = 0x00
    // m4_75=0, m5_15=0, m5_90=0, m6_70=0, m7_40=0, m7_95=0, m10_2=0, m12_2=0

    mrc.m12_2 = 1;
    // mrc.mode = 0x01
    // m4_75=1, m5_15=1, m5_90=1, m6_70=1, m7_40=1, m7_95=1, m10_2=1, m12_2=1
    ```

    Definitely not what we want. Here is an alternative approach:

    ```
    union gsm48_multi_rate_conf_u {
      struct gsm48_multi_rate_conf mrc;
      struct {
        uint8_t hdr;
        uint8_t mode;
      };
    };
    ```

    This should work as expected, but I am fine with the current approach. Keep it as it is.

To view, visit change 29475. To unsubscribe, or for help writing mail filters, visit settings.