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.

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

merged 2 commits into from
Oct 7, 2022

Conversation

fauxpark
Copy link
Member

@fauxpark fauxpark commented Oct 5, 2022

Description

Changed the signature of send_extra to pass the report_extra_t * instead of report ID and usage as separate parameters, so that the common host code does the work of checking the last sent usage and assembling the report struct.

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Keyboard (addition or update)
  • Keymap/layout/userspace (addition or update)
  • Documentation

Issues Fixed or Closed by This PR

Checklist

  • My code follows the code style of this project: C, Python
  • I have read the PR Checklist document and have made the appropriate changes.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have read the CONTRIBUTING document.
  • I have added tests to cover my changes.
  • I have tested the changes and verified that they work and don't break anything (as well as I can manage).

@tzarc tzarc requested a review from a team October 6, 2022 09:58
@drashna drashna merged commit 6dbbeea into qmk:develop Oct 7, 2022
@fauxpark fauxpark deleted the send-extra-refactor branch October 7, 2022 02:36
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).

ramonimbao pushed a commit to ramonimbao/qmk_firmware that referenced this pull request Nov 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants