Skip to content

CDC implementation #344

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

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

CDC implementation #344

wants to merge 3 commits into from

Conversation

hasenbanck
Copy link
Contributor

I ported the CDC implementation from the old USB rework PR to the current core.

Following things were done:

  • Added some timer related components to the core libraries
  • Added the USBSerial library
  • Reworked the variant specific USB files
  • Updated the STM32 USB Device core and CDC libraries
  • Removed some unneeded files

This version works currently fine under RemRam V1 with a baudrate of 115200.

@fpistm It's up to you how to proceed. I of course would like to have these changes as fast as possible upstream, but if you want to rework the general approach anyhow, this PR can also solely as a reference for you later on.

@hasenbanck
CDC implementation
52e1312 
I ported the CDC implementation from the old USB rework PR to the
current core.

Following things were done:
 * Added some timer related components to the core libraries
 * Added the USBSerial library
 * Reworked the variant specific USB files
 * Updated the STM32 USB Device core and CDC libraries
 * Removed some uneeded files

This version works currently fine under RemRam V1 with a baudrate
of 115200.
@fpistm fpistm self-requested a review October 1, 2018 09:15
@ktand
Copy link
Contributor

ktand commented Oct 1, 2018 via email

@hasenbanck
Copy link
Contributor Author

Hi @ktand. Thank you for the input. Are these the changes you mentioned?
ktand@a007e0a

@fpistm I personally don't like that we have those USB files for every board variant, I have a feeling that we could unify them.

@hasenbanck
Add fixes for CDC library
5b7c75d 
Added fixes as advices by @ktand

 * If the USB packet to be sent is equal to the USB buffer size
  (64 bytes), a Zero Length Packet must be sent. Otherwise the
  USB CDC connection will fail.
 * I also added checks for linestate in CDC_Flush and in
  TIM6_PeriodElapsedCallback to ignore transmittion if the
  device is disconnected.
@hasenbanck
Copy link
Contributor Author

Added changes proposed by @ktand .

@romainreignier
Copy link

Hi @hasenbanck
Thank you for working on that feature, it will be really useful for the users of this core.

I have tried quickly last night on the Nucleo144-F429ZI because this board has already the usb directory (I had to made some adjustment according the the REMRAM files). It worked quite well.

Because I have heard that previous attempt to use USB CDC on this core was slow, I have tried the pjrc USB benchmark. The results are not bad for a first version, 32 kB/s, but still far from Teensy performances, so it let some room for improvements. But it is already much better than a plain old serial link so it is already enough to add it in that state.

I also feel that the usb files in each variant directory should be factorize.

Anyway, thanks for your work.

@hasenbanck
Copy link
Contributor Author

@romainreignier Yeah, my main objective is to make this work and make it work correctly. Since I just ported an old approach, speed was not under consideration. I also don't have much experience with the CDC interface, so I don't know what speeds should be theoretical possible.

In my eyes speed improvements could also come later. Especially since this core supports a really broad range of chips.

@fpistm
Copy link
Member

fpistm commented Oct 2, 2018

Thanks for the rebase @hasenbanck
We are totally agree, as I said, in other thread, I think it's possible to avoid dedicated usb files for each variant.
#222 (comment)

Moreover, the configuration in the variant could be more generic and removed from the variant (or partially).

This will be one of the enhancement.

@hasenbanck hasenbanck mentioned this pull request Oct 3, 2018
Closed
@edogaldo
Copy link
Contributor

edogaldo commented Oct 4, 2018

Hi guys, I was working on this topic as well.
I'm missing the point for having duplicate buffers for rx and tx; this is waste of ram, particularly for small devices, is it reallt needed?

Thanks, E.

edogaldo
edogaldo reviewed Oct 4, 2018
View reviewed changes
Copy link
Contributor

@edogaldo edogaldo left a comment •
edited
Loading

Choose a reason for hiding this comment

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

sorry, didn't want to comment on the entire PR...

@edogaldo
Copy link
Contributor

edogaldo commented Oct 4, 2018 •
edited
Loading

Few more comments:

  • what's the purpose for disabling and reenabling the timer each time? ring buffer implementations are generally thread safe (if well implemented)
  • don't know if HID is working but if it is, these changes will break it..
  • if I'm not wrong the Libmaple implementation is not using a timer for TX

edogaldo
edogaldo reviewed Oct 4, 2018
View reviewed changes
cores/arduino/stm32/timer.c
@@ -560,8 +560,8 @@ uint32_t getTimerIrq(TIM_TypeDef* tim)
void TimerHandleDeinit(stimer_t *obj)
{
if(obj != NULL) {
HAL_TIM_Base_DeInit(&(obj->handle));
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should be fixed independently on this PR..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could include it, since @fpistm won't merge this right away. Can you write a patch?

@ktand
Copy link
Contributor

ktand commented Oct 4, 2018 via email

@romainreignier
Copy link

romainreignier commented Oct 4, 2018 via email

@edogaldo
Copy link
Contributor

edogaldo commented Oct 4, 2018

Based on my tests there should be no need for TX buffer at all; this should work just fine:

size_t USBSerial::write(uint8_t c)
{
  return write(&c, 1);
}

size_t USBSerial::write(const uint8_t *buf, size_t len)
{
  if (!(bool) *this || !buf) {
    return 0;
  }

  USBD_CDC_SetTxBuffer(&USBD_Device, (uint8_t *)buf, len);

  uint8_t tResult = USBD_OK;
  do {
    tResult = USBD_CDC_TransmitPacket(&USBD_Device);
  } while (tResult == USBD_BUSY);
  
  return (tResult == USBD_OK ? len : 0);
}

@ktand
Copy link
Contributor

ktand commented Oct 5, 2018 via email

@ktand
Copy link
Contributor

ktand commented Oct 5, 2018 via email

@hasenbanck
Copy link
Contributor Author

@edogaldo @ktand Speed and optimization wasn't the general goal of this PR yet, since I merely rebased an old PR for the CDC feature.

But I like your changes in general. Can you please take my branch and rebase your changes against it? You can then open a PR in my repository against this branch. That way I can easily include your changes.

@hasenbanck
Copy link
Contributor Author

@ktand I tried to add you changes to this branch.

@ppescher ppescher mentioned this pull request Oct 10, 2018
Closed
@fpistm fpistm added enhancement New feature or request New feature labels Oct 12, 2018
@fpistm fpistm mentioned this pull request Oct 12, 2018
Closed
@fpistm fpistm mentioned this pull request Oct 20, 2018
Closed
@fpistm fpistm added this to the 1.5.0 milestone Oct 21, 2018
@fpistm fpistm mentioned this pull request Oct 26, 2018
Closed
@fpistm
Copy link
Member

fpistm commented Nov 9, 2018

I will start next week this feature.
Anyway as mentioned by @edogaldo this PR break the HID part (or at least do not port it as a library).
Based on your work I will split the PR to better track changes and also handle dynamically USB pins.

@hasenbanck
Copy link
Contributor Author

@fpistm Yeah sure, go ahead. This PR was only for discovery / feedback anyhow.

makarenya
makarenya reviewed Nov 11, 2018
View reviewed changes
libraries/USBSerial/src/USBSerial.h
virtual int read(void);

int availableForWrite(void);
virtual size_t write(uint8_t);
Copy link
Contributor

Choose a reason for hiding this comment

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

You overwrite only per-byte write method and use timer resource... But you can overwrite
virtual size_t write(const uint8_t *buffer, size_t size);
method too, and just put received by method buffers into USB... without any timer usage!

Copy link
Contributor

Choose a reason for hiding this comment

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

#344 (comment)

@fpistm
Copy link
Member

fpistm commented Nov 27, 2018

Hi @hasenbanck
did you planned to use a specific vid/pid for RemRam?
You only use the CDC ?

@hasenbanck
Copy link
Contributor Author

Hi @hasenbanck
did you planned to use a specific vid/pid for RemRam?
You only use the CDC ?

@fpistm
No, I don't plan on using a specific vid/pid. I just want to use the CDC feature.

@fpistm
Copy link
Member

fpistm commented Dec 7, 2018 •
edited
Loading

Hi @hasenbanck and all,
Some update:
I should be able to provide an updated PR including HID composite (Keyboard and Mouse) and CDC.
This new PR will be more generic, no more needs to have usb/ directory in the variant. USB pins will be in PeripheralPins.c, usbd_desc and usbd_conf will be common and will handle USB base and USB OTG FS/HS (also FS in HS). This means all MCU with USB capabilities will be handled.
PR will include your remarks and improvements. Anyway several tests will be require.

@fpistm fpistm mentioned this pull request Dec 12, 2018
Merged
@makarenya
Copy link
Contributor

Please, sorry, but... ST usb middleware is not very good option. And it contains some errors in the used version (doesn't work with linux hosts - openwrt as example). I think, that really cool idea - is to port original Arduino attachable usb library for the STM32...

@fpistm
Copy link
Member

fpistm commented Dec 12, 2018

@makarenya,
I'm opened to any proposal and as it is a open source all PR are welcome ;)
This PR is now deprecated and replaced by #388.
ST USB middleware has been updated. I've tested on Linux Ubuntu without any issue.

@fpistm fpistm added the abandoned No more work on this label Dec 12, 2018
@fpistm fpistm self-requested a review December 12, 2018 20:02
@ktand
Copy link
Contributor

ktand commented Dec 13, 2018 via email

@fpistm
Copy link
Member

fpistm commented Dec 13, 2018

Hi, Sorry, the HAL_GPIO_WritePin(GPIOD, GPIO_PIN_10, GPIO_PIN_RESET); is just left over debugging code. Shouldn't be there. Regards, Karl
…
On Thu, Dec 13, 2018 at 8:30 AM Nils Hasenbanck @.> wrote: @.* commented on this pull request. ------------------------------ In libraries/USBSerial/src/usbd_cdc_if.c <#344 (comment)> : > +void CDC_disable_TIM_Interrupt(void) { HAL_NVIC_DisableIRQ(TIM6_DAC_IRQn); } + +void CDC_enable_TIM_Interrupt(void) { HAL_NVIC_EnableIRQ(TIM6_DAC_IRQn); } + +void CDC_resume_receive(void) { + if (receiveSuspended) { + if ((UserRxBufPtrOut + APP_RX_DATA_SIZE - UserRxBufPtrIn - 1) % + APP_RX_DATA_SIZE + + 1 >= + USB_OTG_FS_MAX_PACKET_SIZE) { + USBD_CDC_ReceivePacket( + &hUSBD_Device_CDC); // Initiate next USB packet transfer once a packet + // is received and there is enouch space in the + // buffer + receiveSuspended = false; + HAL_GPIO_WritePin(GPIOD, GPIO_PIN_10, GPIO_PIN_RESET); This change came @ktand https://github.com/ktand 's improvement branch I ported. Maybe it's a change for his particular implementation? Oddly enough it worked on my end without modification. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#344 (comment)>, or mute the thread https://github.com/notifications/unsubscribe-auth/AQ8NJHlHopEy-jiNDhKLCUyTn8BSe_2Pks5u4gIEgaJpZM4XBpya .

No worry. That's what I thought. I've just asked to be sure ;)

fpistm
fpistm reviewed Dec 17, 2018
View reviewed changes
Copy link
Member

@fpistm fpistm left a comment

Choose a reason for hiding this comment

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

This PR is deprecated and replaced by #388 which include HID.
Anyway thanks for the job and all comments, this is greatly appreciated.

@fpistm
Copy link
Member

fpistm commented Dec 21, 2018

I close this one to not pollute the list of PR and bring confusion.
Moreover this one is referenced by #388

@fpistm fpistm closed this Dec 21, 2018
@fpistm fpistm mentioned this pull request Jun 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@makarenya makarenya makarenya left review comments

@edogaldo edogaldo edogaldo left review comments

@fpistm fpistm fpistm left review comments

@kaofishy kaofishy kaofishy left review comments

@xC0000005 xC0000005 xC0000005 left review comments

Assignees
No one assigned
Labels
abandoned No more work on this enhancement New feature or request
Projects
None yet
Milestone
1.5.0
Development

Successfully merging this pull request may close these issues.
8 participants
@hasenbanck @ktand @romainreignier @fpistm @edogaldo @makarenya @kaofishy @xC0000005