ID width question

[algernon]

I started to play around with implementing a hid-io library for Kaleidoscope & QMK, and the id width field in each packet is something I found a bit strange. It is there to allow IDs larger than a byte, yes?

In that case, both the firmware and the device will need to support 8, 16, 24, and 32 bit IDs anyway, and to be space efficient, I'd imagine that both would just store all of these in an uint32_t, otherwise one would need to have four structs and choose the right one, which seems wasteful and needlessly complex.

What if we had 32bit IDs to begin with? That's +3 bytes to transfer over the wire, yeah, but the code to handle things on the firmware side should be a lot simpler. Or, could even restrict it to 16 bits, 64k different commands is not likely to happen anytime soon, I'd imagine. But for the sake of safety, the spec could reserve the 0xffff ID to signal a 32-bit ID.

I think this would make both the wire protocol and the firmware-side implementation a lot simpler.

[haata]

Hmm, so in the firmware I handle using 32 bit as well. Though in theory you could just ignore all packet widths beyond a certain size (say 16 bits) using the width field. I just shift in the value and store as a uint32_t. Even if we just have a 16-bit or 32-bit value shifting will have to occur on some MCUs (endianness) if they don't have the endian swap instructions (Cortex M4 does).

The problem using a large id width right away is that 8 byte packets would only have 2 byte payloads which would be really chatty and slow (USB 2.0 Low-Speed). I've ended up simplifying the continued packets to include the Id as well so you don't get that extra byte savings. Alternatively we could drop support for USB 2.0 Low-Speed. This would put us at 6 bytes of header per packet (a few bits for control, 1 byte + change for length, 4 bytes for id). I want to support USB 2.0 HS, so we need at least a 1024'ish length.

As for extensions, we'd need multiple structs to handle this and that kinda defeats simplification.

I'll let you see my implementation, but I handle packets differently depending on if they are incoming or outgoing. Outgoing, I store the packets as ready to send. Incoming I store them as contiguous buffers of payload with a header indicating the state of the overall message (in the case of continued packets) so I can just deal with a chunk of data.

Let me sleep on this, I'm not really sure what's ideal.

[haata]

Thinking more on this I think I like your idea for doing 16 bit Ids, with the option of moving to 32 bit in the future.

The packet format would look something like this.

struct HIDIO_Packet {
    uint8_t packet_type:3;
    uint8_t cont:1;
    uint8_t width:1; = 0
    uint8_t reserved:1;
    uint8_t upper_len:2;
    uint8_t len;
    uint16_t id;
    uint8_t payload[0];
}

struct HIDIO_Packet32 {
    uint8_t packet_type:3;
    uint8_t cont:1;
    uint8_t width:1; // = 1
    uint8_t reserved:1;
    uint8_t upper_len:2;
    uint8_t len;
    uint32_t id;
    uint8_t payload[0];
}

For now, all we'd have to do is validate that the width is set to 0, and we can assume that struct. If we ever need to go beyond 65k Ids (lol), we can deal with that by adding an additional struct.

Thoughts?

[algernon]

Sounds good! Minor nitpick, but perhaps rename width to id_width_32 or id_width to better describe what the bit is for?

[haata]

Reasonable suggestion, consider it done.

I'm going to finish the firmware implementation with the old Id style first (nearly have the packet stuff working), then I'll convert to the new style before working on the Rust packet code.

[haata]

Finished conversion to the new packet format style. Since I already had all the code to handle it, already supporting both 16 and 32 bit widths.

https://github.com/kiibohd/controller/commit/7ba333793abb6a841248dafda3aa2d5deff5c06b

I ended up adding one more struct. Turns out having a generic no Id version is quite useful.

typedef struct HIDIO_Packet {
    HIDIO_Packet_Type type:3;
    uint8_t           cont:1;      // 0 - Only packet, 1 continued packet following
    uint8_t           id_width:1;  // 0 - 16bits, 1 - 32bits
    uint8_t           reserved:1;  // Reserved
    uint8_t           upper_len:2; // Upper 2 bits of length field (generally unused)
    uint8_t           len;         // Lower 8 bits of length field
    uint8_t           data[0];     // Start of data payload (may start with Id)
} __attribute((packed)) HIDIO_Packet;

typedef struct HIDIO_Packet16 {
    HIDIO_Packet_Type type:3;
    uint8_t           cont:1;      // 0 - Only packet, 1 continued packet following
    uint8_t           id_width:1;  // 0 - 16bits, 1 - 32bits
    uint8_t           reserved:1;  // Reserved
    uint8_t           upper_len:2; // Upper 2 bits of length field (generally unused)
    uint8_t           len;         // Lower 8 bits of length field
    uint16_t          id;          // Id field (check id_width to see which struct to use)
    uint8_t           data[0];     // Start of data payload
} __attribute((packed)) HIDIO_Packet16;

typedef struct HIDIO_Packet32 {
    HIDIO_Packet_Type type:3;
    uint8_t           cont:1;      // 0 - Only packet, 1 continued packet following
    uint8_t           id_width:1;  // 0 - 16bits, 1 - 32bits
    uint8_t           reserved:1;  // Reserved
    uint8_t           upper_len:2; // Upper 2 bits of length field (generally unused)
    uint8_t           len;         // Lower 8 bits of length field
    uint32_t          id;          // Id field (check id_width to see which struct to use)
    uint8_t           data[0];     // Start of data payload
} __attribute((packed)) HIDIO_Packet32;

Unit tests are passing: https://travis-ci.org/kiibohd/controller/jobs/252510274#L2008

[algernon]

My questions have been answered, and am happy with the outcome, so closing this. Thanks!