[Refactor] Vocabulary and consistency for HID reports

[amrsoll]

EDIT: Nothing to see here, everything is considered for XAP

I want to disambiguate the words/variables/functions that make use of report, data and usage to describe the state of switches, HID reports and their implementation for the different platforms/drivers.

I am also hoping to get comments on how wrong / right I am about the analysis.

⚠️ This is either partially or completely redundant with the introduction of XAP

TL;DR;

In the name of separation of concerns, I propose refactoring of the HID report datastructures and their respective send_*() functions.

In particular, I am suggesting the usage of these terms and conventions for their specific cases

• hid_report_foo_t to describe a struct that contains at least uint8_t .report_id

  Example

  // report.h
  typedef struct {
     uint8_t  report_id;
     uint8_t attr1;
     uint16_t attr2;
     // ...
  } __attribute__((packed)) hid_report_foo_t;

  Ideally hid_report_foo would be a subset/implementation of a hid_report (like an abstract base class)

• Use descriptive names for the attributes of hid_report_foo_t

  Example

  instead of data or usage, use switch_state, closed_switches, reported_keys or pressed_btns

  // report.h
  typedef struct {
      uint8_t  report_id;
      uint16_t pressed_keys; // instead of 'usage'
  } __attribute__((packed)) hid_report_extra_t;

• host_foo_send() and send_foo() have to use hid_report_foo as argument

  Example

  static void send_foo(hid_report_foo_t *hid_report);

⚠️ The changes to functions arguments would be breaking / non backwards compatible

Vocabulary confusion between different abstractions

Inconsistent vocabulary ...

The words report, data and usage are sometimes used interchangeably, but not exclusively for a specific usage. Because they bleed out onto different data structures and meanings, they can obfuscate each others purpose.

Examples

// report.h
typedef struct {
    uint8_t  report_id;
    uint32_t usage;
} __attribute__((packed)) report_programmable_button_t;

// programmable_button.c
#define REPORT_BIT(index) (((uint32_t)1) << (index - 1))
static uint32_t programmable_button_report = 0;

void programmable_button_on(uint8_t index) {
    programmable_button_report |= REPORT_BIT(index);
}
void programmable_button_send(void) {
    host_programmable_button_send(programmable_button_report);
}

// host.c
void host_programmable_button_send(uint32_t report) {
    // [...]
    (*driver->send_programmable_button)(report);
}

// vusb.c
// Here there is additionally the word `data` used
static void send_programmable_button(uint32_t data) {
    static report_programmable_button_t report = {
        .report_id = REPORT_ID_PROGRAMMABLE_BUTTON,
    };
    report.usage = data;
    // [...]
    usbSetInterruptShared((void *)&report, sizeof(report));
}

And in the case of send_consumer():

typedef struct {
    uint8_t  report_id;
    uint16_t usage;
} __attribute__((packed)) report_extra_t;

// usb_main.c
static void send_extra(uint8_t report_id, uint16_t data) {
    // [...]
    static report_extra_t report;
    report = (report_extra_t){.report_id = report_id, .usage = data};

    usbStartTransmitI(&USB_DRIVER, SHARED_IN_EPNUM, (uint8_t *)&report, sizeof(report_extra_t));
    // [...]
}

void send_consumer(uint16_t data) {
    send_extra(REPORT_ID_CONSUMER, data);
}

// host.c
static uint16_t last_consumer_report = 0;
void host_consumer_send(uint16_t report) {
    if (report == last_consumer_report) return;
    last_consumer_report = report;

    // [...]
    (*driver->send_consumer)(report);
}

... leading to mismatching interfaces ...

If a report has a single field (outside of .report_id, then the field is used interchangeably/liberaly instead of being wrapped in the report struct. The reason might be to cut down on overall lines of code but I don't think it helps with the size of the final binary.

This means that all the driver interfaces have to know the underlying data structure and size to forward the call. Changes to data type and structure is therefore made more difficult.

Examples

send_* functions:

// chibios.c

/* declarations */
uint8_t keyboard_leds(void);
void    send_keyboard(report_keyboard_t *report);
void    send_mouse(report_mouse_t *report);
void    send_system(uint16_t data);
void    send_consumer(uint16_t data);
void    send_programmable_button(uint32_t data);
void    send_digitizer(report_digitizer_t *report);

// host.h

/* host driver interface */
uint8_t host_keyboard_leds(void);
led_t   host_keyboard_led_state(void);
void    host_keyboard_send(report_keyboard_t *report);
void    host_mouse_send(report_mouse_t *report);
void    host_system_send(uint16_t data);
void    host_consumer_send(uint16_t data);
void    host_programmable_button_send(uint32_t data);

... and different implementations

Retracted

This means the HID report is either created on the device driver side (tmk_core/protocol), or on the side of the key processor (quantum/), which is also an inconsistent behaviour.

I suggest to have all the HID reports

1. generated in the key processors (quantum/)
2. Forwarded through the appropriate channels (sometimes through host driver)
3. Final drivers read the HID report and send packets

Example

• ~~The mouse HID report is generated in quantum/pointing_device.c ~~
• The extra keys (EXTRA_KEYS_ENABLE) using host_system_send() or host_consumer_send() have their HID report generated on the device driver, e.g. tmk_core/protocol/arm_atsam/main_arm_atsam.c

Using non-descriptive variable names

When referencing the state of the switches, the word data is often used as a stopgap term for single attribute hid reports, especially for driver implementations. At that stage the data does have a meaning and can benefit from a specific term. Among other benefits, increased visibility makes it easier to lookup other definitions/usage of that term.

See : Article written against the usage of vague terms / names

Additionally, the attribute could be assigned a typedef if the type could be foreseen to change (as it was for layer_state_t). Changing the size of the variable type could all be done from report.h. Every driver implementation probably needs to be updated anyway.

Example

// report.h
typedef attr_1_foo_t uint16_t

typedef struct {
    uint8_t  report_id;
    attr_1_foo_t attr1;
} __attribute__((packed)) hid_report_foo_t;

// host.c
static attr_1_foo_t last_foo_attr_1 = 0

void host_foo_send(attr_1_foo_t attr_1) {
    if (attr_1 == last_foo_attr_1) return;
    last_foo_attr_1 = attr_1;

    if (!driver) return;
    (*driver->send_foo)(usage);
}

Finally

I apologise if the over-documentation comes across as condescending, but I prefer to specify goal, argumentation and examples as much as possible; even when discussing with strangers of the internet that are way more knowledgeable than myself.

I would be happy to make those changes if they are deemed reasonable.

EDIT: I made a mistake about assuming the HID report was generated from key processors in quantum/

[amrsoll]

Closing while I update the issue content ( misunderstood the code )