Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor send_extra #18615

Merged
merged 2 commits into from
Oct 7, 2022
Merged

Refactor send_extra #18615

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4f3cf23
Refactor `send_extra`
fauxpark Oct 5, 2022
31afa97
format
fauxpark Oct 5, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions keyboards/annepro2/annepro2_ble.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
/* -------------------- Static Function Prototypes -------------------------- */
static uint8_t ap2_ble_leds(void);
static void ap2_ble_mouse(report_mouse_t *report);
static void ap2_ble_extra(uint8_t report_id, uint16_t data);
static void ap2_ble_extra(report_extra_t *report);
static void ap2_ble_keyboard(report_keyboard_t *report);

static void ap2_ble_swtich_ble_driver(void);
Expand Down Expand Up @@ -149,11 +149,11 @@ static inline uint16_t CONSUMER2AP2(uint16_t usage) {
}
}

static void ap2_ble_extra(uint8_t report_id, uint16_t data) {
if (report_id == REPORT_ID_CONSUMER) {
static void ap2_ble_extra(report_extra_t *report) {
if (report->report_id == REPORT_ID_CONSUMER) {
sdPut(&SD1, 0x0);
sdWrite(&SD1, ble_mcu_send_consumer_report, sizeof(ble_mcu_send_consumer_report));
sdPut(&SD1, CONSUMER2AP2(data));
sdPut(&SD1, CONSUMER2AP2(report->usage));
static const uint8_t dummy[3] = {0};
sdWrite(&SD1, dummy, sizeof(dummy));
}
Expand Down
13 changes: 4 additions & 9 deletions keyboards/bioi/ble.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -96,7 +96,7 @@ static void bluefruit_serial_send(uint8_t data)
static uint8_t keyboard_leds(void);
static void send_keyboard(report_keyboard_t *report);
static void send_mouse(report_mouse_t *report);
static void send_extra(uint8_t report_id, uint16_t data);
static void send_extra(report_extra_t *report);

host_driver_t bluefruit_driver = {
keyboard_leds,
Expand Down Expand Up @@ -177,15 +177,10 @@ static void send_mouse(report_mouse_t *report)
#define CONSUMER2BLUEFRUIT(usage) \
(usage == AUDIO_MUTE ? 0x00e2 : (usage == AUDIO_VOL_UP ? 0x00e9 : (usage == AUDIO_VOL_DOWN ? 0x00ea : (usage == TRANSPORT_NEXT_TRACK ? 0x00b5 : (usage == TRANSPORT_PREV_TRACK ? 0x00b6 : (usage == TRANSPORT_STOP ? 0x00b7 : (usage == TRANSPORT_STOP_EJECT ? 0x00b8 : (usage == TRANSPORT_PLAY_PAUSE ? 0x00b1 : (usage == AL_CC_CONFIG ? 0x0183 : (usage == AL_EMAIL ? 0x018c : (usage == AL_CALCULATOR ? 0x0192 : (usage == AL_LOCAL_BROWSER ? 0x0196 : (usage == AC_SEARCH ? 0x021f : (usage == AC_HOME ? 0x0223 : (usage == AC_BACK ? 0x0224 : (usage == AC_FORWARD ? 0x0225 : (usage == AC_STOP ? 0x0226 : (usage == AC_REFRESH ? 0x0227 : (usage == AC_BOOKMARKS ? 0x022a : 0)))))))))))))))))))

static void send_extra(uint8_t report_id, uint16_t data)
static void send_extra(report_extra_t *report)
{
if (report_id == REPORT_ID_CONSUMER) {
static uint16_t last_data = 0;
if (data == last_data)
return;
last_data = data;

uint16_t bitmap = CONSUMER2BLUEFRUIT(data);
if (report->report_id == REPORT_ID_CONSUMER) {
uint16_t bitmap = CONSUMER2BLUEFRUIT(report->usage);

#ifdef BLUEFRUIT_TRACE_SERIAL
dprintf("\nData: ");
Expand Down
12 changes: 6 additions & 6 deletions keyboards/hhkb/rn42/rn42.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
static uint8_t keyboard_leds(void);
static void send_keyboard(report_keyboard_t *report);
static void send_mouse(report_mouse_t *report);
static void send_extra(uint8_t report_id, uint16_t data);
static void send_extra(report_extra_t *report);

host_driver_t rn42_driver = {
keyboard_leds,
Expand Down Expand Up @@ -221,10 +221,10 @@ static uint16_t usage2bits(uint16_t usage)
}


static void send_extra(uint8_t report_id, uint16_t data)
static void send_extra(report_extra_t *report)
{
if (report_id == REPORT_ID_CONSUMER) {
uint16_t bits = usage2bits(data);
if (report->report_id == REPORT_ID_CONSUMER) {
uint16_t bits = usage2bits(report->usage);
serial_send(0xFD); // Raw report mode
serial_send(3); // length
serial_send(3); // descriptor type
Expand All @@ -238,7 +238,7 @@ static void send_extra(uint8_t report_id, uint16_t data)
static uint8_t config_keyboard_leds(void);
static void config_send_keyboard(report_keyboard_t *report);
static void config_send_mouse(report_mouse_t *report);
static void config_send_extra(uint8_t report_id, uint16_t data);
static void config_send_extra(report_extra_t *report);

host_driver_t rn42_config_driver = {
config_keyboard_leds,
Expand All @@ -250,4 +250,4 @@ host_driver_t rn42_config_driver = {
static uint8_t config_keyboard_leds(void) { return leds; }
static void config_send_keyboard(report_keyboard_t *report) {}
static void config_send_mouse(report_mouse_t *report) {}
static void config_send_extra(uint8_t report_id, uint16_t data) {}
static void config_send_extra(report_extra_t *report) {}
4 changes: 2 additions & 2 deletions tests/test_common/test_driver.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,8 +53,8 @@ void TestDriver::send_mouse(report_mouse_t* report) {
m_this->send_mouse_mock(*report);
}

void TestDriver::send_extra(uint8_t report_id, uint16_t data) {
m_this->send_extra_mock(report_id, data);
void TestDriver::send_extra(report_extra_t* report) {
m_this->send_extra_mock(*report);
}

namespace internal {
Expand Down
4 changes: 2 additions & 2 deletions tests/test_common/test_driver.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,13 +32,13 @@ class TestDriver {

MOCK_METHOD1(send_keyboard_mock, void(report_keyboard_t&));
MOCK_METHOD1(send_mouse_mock, void(report_mouse_t&));
MOCK_METHOD2(send_extra_mock, void(uint8_t, uint16_t));
MOCK_METHOD1(send_extra_mock, void(report_extra_t&));

private:
static uint8_t keyboard_leds(void);
static void send_keyboard(report_keyboard_t* report);
static void send_mouse(report_mouse_t* report);
static void send_extra(uint8_t report_id, uint16_t data);
static void send_extra(report_extra_t* report);
host_driver_t m_driver;
uint8_t m_leds = 0;
static TestDriver* m_this;
Expand Down
9 changes: 4 additions & 5 deletions tmk_core/protocol/arm_atsam/main_arm_atsam.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ void main_subtasks(void);
uint8_t keyboard_leds(void);
void send_keyboard(report_keyboard_t *report);
void send_mouse(report_mouse_t *report);
void send_extra(uint8_t report_id, uint16_t data);
void send_extra(report_extra_t *report);

#ifdef DEFERRED_EXEC_ENABLE
void deferred_exec_task(void);
Expand Down Expand Up @@ -113,17 +113,16 @@ void send_mouse(report_mouse_t *report) {
#endif // MOUSEKEY_ENABLE
}

void send_extra(uint8_t report_id, uint16_t data) {
void send_extra(report_extra_t *report) {
#ifdef EXTRAKEY_ENABLE
uint32_t irqflags;

irqflags = __get_PRIMASK();
__disable_irq();
__DMB();

udi_hid_exk_report.desc.report_id = report_id;
udi_hid_exk_report.desc.report_data = data;
udi_hid_exk_b_report_valid = 1;
memcpy(udi_hid_exk_report, report, UDI_HID_EXK_REPORT_SIZE);
udi_hid_exk_b_report_valid = 1;
udi_hid_exk_send_report();

__DMB();
Expand Down
14 changes: 1 addition & 13 deletions tmk_core/protocol/arm_atsam/usb/udi_device_conf.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,21 +352,9 @@ typedef struct {

// clang-format on

// set report buffer (from host)
extern uint8_t udi_hid_exk_report_set;

// report buffer
# define UDI_HID_EXK_REPORT_SIZE 3

typedef union {
struct {
uint8_t report_id;
uint16_t report_data;
} desc;
uint8_t raw[UDI_HID_EXK_REPORT_SIZE];
} udi_hid_exk_report_t;

extern udi_hid_exk_report_t udi_hid_exk_report;
extern uint8_t udi_hid_exk_report[UDI_HID_EXK_REPORT_SIZE];

COMPILER_PACK_RESET()

Expand Down
29 changes: 6 additions & 23 deletions tmk_core/protocol/arm_atsam/usb/udi_hid_kbd.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -371,13 +371,13 @@ static uint8_t udi_hid_exk_rate;
COMPILER_WORD_ALIGNED
static uint8_t udi_hid_exk_protocol;

COMPILER_WORD_ALIGNED
uint8_t udi_hid_exk_report_set;
// COMPILER_WORD_ALIGNED
// uint8_t udi_hid_exk_report_set;

bool udi_hid_exk_b_report_valid;

COMPILER_WORD_ALIGNED
udi_hid_exk_report_t udi_hid_exk_report;
uint8_t udi_hid_exk_report[UDI_HID_EXK_REPORT_SIZE];

static bool udi_hid_exk_b_report_trans_ongoing;

Expand Down Expand Up @@ -415,39 +415,24 @@ UDC_DESC_STORAGE udi_hid_exk_report_desc_t udi_hid_exk_report_desc = {{
//clang-format on
}};

static bool udi_hid_exk_setreport(void);

static void udi_hid_exk_report_sent(udd_ep_status_t status, iram_size_t nb_sent, udd_ep_id_t ep);

static void udi_hid_exk_setreport_valid(void);

bool udi_hid_exk_enable(void) {
// Initialize internal values
udi_hid_exk_rate = 0;
udi_hid_exk_protocol = 0;
udi_hid_exk_b_report_trans_ongoing = false;
memset(udi_hid_exk_report.raw, 0, UDI_HID_EXK_REPORT_SIZE);
memset(udi_hid_exk_report, 0, UDI_HID_EXK_REPORT_SIZE);
udi_hid_exk_b_report_valid = false;
return UDI_HID_EXK_ENABLE_EXT();
}

void udi_hid_exk_disable(void) { UDI_HID_EXK_DISABLE_EXT(); }

bool udi_hid_exk_setup(void) { return udi_hid_setup(&udi_hid_exk_rate, &udi_hid_exk_protocol, (uint8_t *)&udi_hid_exk_report_desc, udi_hid_exk_setreport); }
bool udi_hid_exk_setup(void) { return udi_hid_setup(&udi_hid_exk_rate, &udi_hid_exk_protocol, (uint8_t *)&udi_hid_exk_report_desc, NULL); }

uint8_t udi_hid_exk_getsetting(void) { return 0; }

static bool udi_hid_exk_setreport(void) {
if ((USB_HID_REPORT_TYPE_OUTPUT == (udd_g_ctrlreq.req.wValue >> 8)) && (0 == (0xFF & udd_g_ctrlreq.req.wValue)) && (1 == udd_g_ctrlreq.req.wLength)) {
// Report OUT type on report ID 0 from USB Host
udd_g_ctrlreq.payload = &udi_hid_exk_report_set;
udd_g_ctrlreq.callback = udi_hid_exk_setreport_valid;
udd_g_ctrlreq.payload_size = 1;
return true;
}
return false;
}

bool udi_hid_exk_send_report(void) {
if (!main_b_exk_enable) {
return false;
Expand All @@ -457,7 +442,7 @@ bool udi_hid_exk_send_report(void) {
return false;
}

memcpy(udi_hid_exk_report_trans, udi_hid_exk_report.raw, UDI_HID_EXK_REPORT_SIZE);
memcpy(udi_hid_exk_report_trans, udi_hid_exk_report, UDI_HID_EXK_REPORT_SIZE);
udi_hid_exk_b_report_valid = false;
udi_hid_exk_b_report_trans_ongoing = udd_ep_run(UDI_HID_EXK_EP_IN | USB_EP_DIR_IN, false, udi_hid_exk_report_trans, UDI_HID_EXK_REPORT_SIZE, udi_hid_exk_report_sent);

Expand All @@ -474,8 +459,6 @@ static void udi_hid_exk_report_sent(udd_ep_status_t status, iram_size_t nb_sent,
}
}

static void udi_hid_exk_setreport_valid(void) {}

#endif // EXTRAKEY_ENABLE

//********************************************************************************************
Expand Down
1 change: 0 additions & 1 deletion tmk_core/protocol/arm_atsam/usb/udi_hid_kbd.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,6 @@ bool udi_hid_nkro_send_report(void);
#ifdef EXTRAKEY_ENABLE
extern UDC_DESC_STORAGE udi_api_t udi_api_hid_exk;
extern bool udi_hid_exk_b_report_valid;
extern uint8_t udi_hid_exk_report_set;
bool udi_hid_exk_send_report(void);
#endif // EXTRAKEY_ENABLE

Expand Down
2 changes: 1 addition & 1 deletion tmk_core/protocol/chibios/chibios.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@
uint8_t keyboard_leds(void);
void send_keyboard(report_keyboard_t *report);
void send_mouse(report_mouse_t *report);
void send_extra(uint8_t report_id, uint16_t data);
void send_extra(report_extra_t *report);

/* host struct */
host_driver_t chibios_driver = {keyboard_leds, send_keyboard, send_mouse, send_extra};
Expand Down
7 changes: 2 additions & 5 deletions tmk_core/protocol/chibios/usb_main.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -943,7 +943,7 @@ void shared_in_cb(USBDriver *usbp, usbep_t ep) {
* ---------------------------------------------------------
*/

void send_extra(uint8_t report_id, uint16_t data) {
void send_extra(report_extra_t *report) {
#ifdef EXTRAKEY_ENABLE
osalSysLock();
if (usbGetDriverStateI(&USB_DRIVER) != USB_ACTIVE) {
Expand All @@ -962,10 +962,7 @@ 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));
usbStartTransmitI(&USB_DRIVER, SHARED_IN_EPNUM, (uint8_t *)report, sizeof(report_extra_t));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the report pointer to usbStartTransmitI() like that requires that the report is kept valid until it actually gets sent (which may happen asynchronously after quite some time), and apparently host_system_send() and host_consumer_send() don't ensure that — they just pass a pointer to some data on the stack and don't wait until the actual transmission completes.

However, this code may still pretend to work on many hardware implementations (STM32 with non-OTG USB controllers, RP2040, HT32, KINETIS, LPC11Uxx, NUC123, SN32, WB32), because the underlying implementation of usb_lld_start_in() copies at least the data for the first USB packet synchronously, and this report always fits into one USB packet. But it may fail on STM32 with the OTG controller (e.g., F4x1) or GD32VF103, because the USB drivers for those chips copy the data even for the first USB packet of the transfer inside the USB interrupt handler, so this copying has a chance to happen after the report data gets invalidated (but it might be hard to actually make this happen, because the FIFO IRQ probably gets asserted immediately after usb_lld_start_in() writes into the USB controller registers, therefore by the time the execution reaches the osalSysUnlock() below the USB IRQ is already pending and will get served immediately).

Looks like only the ChibiOS implementation has this problem (arm_atsam and VUSB copy the data into some buffer, LUFA synchronously copies the data into the USB controller buffer).

osalSysUnlock();
#endif
}
Expand Down
40 changes: 25 additions & 15 deletions tmk_core/protocol/host.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,8 +41,8 @@ extern keymap_config_t keymap_config;
#endif

static host_driver_t *driver;
static uint16_t last_system_report = 0;
static uint16_t last_consumer_report = 0;
static uint16_t last_system_usage = 0;
static uint16_t last_consumer_usage = 0;

void host_set_driver(host_driver_t *d) {
driver = d;
Expand Down Expand Up @@ -126,27 +126,37 @@ void host_mouse_send(report_mouse_t *report) {
(*driver->send_mouse)(report);
}

void host_system_send(uint16_t report) {
if (report == last_system_report) return;
last_system_report = report;
void host_system_send(uint16_t usage) {
if (usage == last_system_usage) return;
last_system_usage = usage;

if (!driver) return;
(*driver->send_extra)(REPORT_ID_SYSTEM, report);

report_extra_t report = {
.report_id = REPORT_ID_SYSTEM,
.usage = usage,
};
(*driver->send_extra)(&report);
}

void host_consumer_send(uint16_t report) {
if (report == last_consumer_report) return;
last_consumer_report = report;
void host_consumer_send(uint16_t usage) {
if (usage == last_consumer_usage) return;
last_consumer_usage = usage;

#ifdef BLUETOOTH_ENABLE
if (where_to_send() == OUTPUT_BLUETOOTH) {
bluetooth_send_consumer(report);
bluetooth_send_consumer(usage);
return;
}
#endif

if (!driver) return;
(*driver->send_extra)(REPORT_ID_CONSUMER, report);

report_extra_t report = {
.report_id = REPORT_ID_CONSUMER,
.usage = usage,
};
(*driver->send_extra)(&report);
}

#ifdef JOYSTICK_ENABLE
Expand Down Expand Up @@ -232,10 +242,10 @@ void host_programmable_button_send(uint32_t data) {

__attribute__((weak)) void send_programmable_button(report_programmable_button_t *report) {}

uint16_t host_last_system_report(void) {
return last_system_report;
uint16_t host_last_system_usage(void) {
return last_system_usage;
}

uint16_t host_last_consumer_report(void) {
return last_consumer_report;
uint16_t host_last_consumer_usage(void) {
return last_consumer_usage;
}
8 changes: 4 additions & 4 deletions tmk_core/protocol/host.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,12 @@ 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_system_send(uint16_t usage);
void host_consumer_send(uint16_t usage);
void host_programmable_button_send(uint32_t data);

uint16_t host_last_system_report(void);
uint16_t host_last_consumer_report(void);
uint16_t host_last_system_usage(void);
uint16_t host_last_consumer_usage(void);

#ifdef __cplusplus
}
Expand Down
2 changes: 1 addition & 1 deletion tmk_core/protocol/host_driver.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ typedef struct {
uint8_t (*keyboard_leds)(void);
void (*send_keyboard)(report_keyboard_t *);
void (*send_mouse)(report_mouse_t *);
void (*send_extra)(uint8_t, uint16_t);
void (*send_extra)(report_extra_t *);
} host_driver_t;

void send_joystick(report_joystick_t *report);
Expand Down
Loading