Specify endianness

[jamwaffles]

I'm writing a driver for the MAX17302 that uses 16 bit words over an I2C interface. Page 140 of the datasheet states:

With 2-Wire communication, a byte of data consists of 8 bits ordered most significant bit (MSb) first. The least significant bit (LSb) of each byte is followed by the Acknowledge bit. IC registers composed of multibyte values are ordered least significant byte (LSB) first.

The load_be used in the implement_register_field macro seems to be loading the values returned over the wire incorrectly. I'm using full-width fields like so:

// Create a register set for the device
implement_registers!(
    /// The global register set
    Max17302.registers<u16> = {
        avgcurrent(RW, 0x01D, 2) = { avgcurrent: u16 = RW 0..16 },
    }
);

I forked device-driver and changed the two occurrences of load_be to load_le in implement_register_field and now the values are returned as expected.

Should it be possible to specify the endianness of device-driver, or is load_be a bug?

[diondokter]

Right, I see! I definitely see why little endian is wanted, but I'd like BE to remain the default. Do we want to be able to select lsb vs msb as well?

Maybe I could make something like this:

// Create a register set for the device
implement_registers!(
    /// The global register set
    Max17302.registers<u16> = {
        avgcurrent(RW, 0x01D, 2) = MSB { avgcurrent: u16(LE) = RW 0..16 },
        //                         ^^^                   ^^
        //         optional. default = LSB           optional. default = BE (maybe also support native endiannes?)
    }
);

Would this be good for what you want? Maybe you have a better suggestion 😄

LSB seems like a reasonable default. (It is already)

BE is also the best default IMO because this is a bit-specific interface. The bits you give are from the start of an array to the end. LE has weird ordering that behaves differently from this.

For example, if you have a 10-bit field that you capture in an enum for example, then I'd want to encode it like so:

enum X {
    Bit0Set = 0b0000000001,
    Bit9Set = 0b1000000000,
}

If this were loaded as in LE, then it would have been specified as

enum X {
    Bit0Set = 0b0000000100,
    Bit9Set = 0b0000000010,
}

At least... I think... Normally fields are just in bit order like the first example.

[jamwaffles]

I definitely see why little endian is wanted, but I'd like BE to remain the default.

Of course! I'm not suggesting we change the default, just the ability to change the endianness :).

I think the issue I'm running into is much closer to the wire than the register definitions themselves. My read_register impl looks like this:

fn read_register(
    &mut self,
    address: Self::Address,
    buffer: &mut [u8],
) -> Result<(), Self::InterfaceError> {
    let Addresses {
        slave_addr,
        register_addr,
    } = Self::addresses(address);

    self.comm
        .write_read(slave_addr, &[register_addr], buffer)
        .map_err(|_| InterfaceError::Comm)?;

    Ok(())
}

which gives me no control over the endianness of what's written into buffer. Would it be better to focus on allowing the endianness and/or MSB/LSB to be configured at this point perhaps?

In writing this reply, it just occurred to me that I can simple call buffer.reverse() in read_register which fixes my original issue. It doesn't feel particularly clean, but I'm happy for this issue to be closed if you would prefer to keep device-driver simpler.

[diondokter]

@jamwaffles I've done some work and I think this should be able to do it.

You can now make your field be read in little endian format like so:

// Create a register set for the device
implement_registers!(
    /// The global register set
    Max17302.registers<u16> = {
        avgcurrent(RW, 0x01D, 2) = { avgcurrent: u16:LE = RW 0..16 },
        //                                          ^^^ add this
    }
);

Would appreciate if you could try this out (see the PR, the branch is bit_orders) and let me know if this works for you.

[diondokter]

Hey @jamwaffles do you think you've got an opportunity to test this out at some point?

If not, then that's ok, but then I won't wait for you and just merge it 🙂. I think it works fine how it is.

[jamwaffles]

Hey, sorry to keep you waiting! I don't have a lot of time to test this right now, but for my use case specifically I'd want to set the LE/BE for all registers once as they all use the same ordering over the wire. I guess for now it'd be fine defining LE/BE for each register individually. A global override can always be added later.

I got round the original problem by modifying my RegisterInterface implementation to combine the bytes from the wire in the opposite order, so I'm pretty easy on any specific implementation of endianness support.