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

Add notification support for device class USBTMC #2494

Merged
merged 6 commits into from Apr 28, 2024

Conversation

tommie
Copy link
Contributor

@tommie tommie commented Feb 26, 2024

The ep_int_in is already used for responding to USB488 READ_STATUS_BYTE requests, but that EP is defined for the entire USBTMC class. This extends the functionality to let callers send notifications and receive ACKs.

Tested on an RP2040.

The ep_int_in is already used for responding to USB488
READ_STATUS_BYTE requests, but that EP is defined for all of USBTMC.
This extends the functionality to let callers send notifications and
receive ACKs.
#endif
if (usbd_edpt_busy(usbtmc_state.rhport, usbtmc_state.ep_int_in)) return false;

TU_VERIFY(usbd_edpt_xfer(usbtmc_state.rhport, usbtmc_state.ep_int_in, (void *)(uintptr_t) data, len));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed the failing constness check by casting to uintptr_t, as done in other places.

@HiFiPhile
Copy link
Collaborator

Not a TMC expert, just to mention there is no buffering inside usbd_edpt_xfer(), you need to add buffer like how bulk endpoints did:

CFG_TUSB_MEM_ALIGN uint8_t ep_bulk_in_buf[USBTMCD_BUFFER_SIZE];

@tommie
Copy link
Contributor Author

tommie commented Mar 15, 2024

Not a TMC expert, just to mention there is no buffering inside usbd_edpt_xfer(), you need to add buffer like how bulk endpoints did:

CFG_TUSB_MEM_ALIGN uint8_t ep_bulk_in_buf[USBTMCD_BUFFER_SIZE];

Yes, I documented that the buffer must remain valid until the notification_complete callback, since that's what tud_usbtmc_transmit_dev_msg_data already does:

https://github.com/hathach/tinyusb/pull/2494/files#diff-0c276774d2aac9050bffd9993a595ad46c6e04638f9f2aaa9fb0c5ab76b973eaR100

Re. TMC, I had to ditch it for two reasons:

  1. WebUSB can't release the kernel driver, so claiming the interface requires OS configuration shenanigans. (And according to WebUSB folks, doing more is out of scope for WebUSB.)
  2. I'd love to change the TinyUSB TMC API to not require the full message to tud_usbtmc_transmit_dev_msg_data: if the bulk-IN message is composed of multiple parts located in different parts of memory, having to duplicate them all at once just for transfer seems wasteful. Especially since USBTMC only needs enough buffer for one USB transfer at a time.

In the end, it seemed TMC as a spec was more in the way than useful.

@HiFiPhile HiFiPhile requested a review from pigrew April 26, 2024 19:24
@HiFiPhile
Copy link
Collaborator

I just realized the lacking of buffer copying in bulk IN transfer is problematic, as some MCU require buffer to be placed into a special RAM region and has alignment requirement. To ensure compatibility I had to add it.

For this reason I've added a buffer for INT transfer with size configured by CFG_TUD_USBTMC_INT_EP_SIZE.

@HiFiPhile HiFiPhile merged commit 1c04d59 into hathach:master Apr 28, 2024
69 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants