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

cdcacm: send RX flow control only for the first time when limit is crossed #7558

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

michallenc
Copy link
Contributor

Summary

Current implementation sends RX flow control activation every time the limit is crossed when receive packet is read by driver. This may result in repetitious sends of RX flow control signal when some packets are left on the bus or in FIFO.

This change ensures RX flow control is send only when it is not active (priv->iactive is false). It also removes break statement from while loop when watermark is used. The driver should activate RX flow and read the rest of the packet instead of discarding it.

I am not entirely sure if this is the correct way to implement this, should be carefully checked before merged (cc to @gregory-nutt who implemented the flow control to CDC ACM).

Impact

CDC ACM device driver.

Testing

Tested on SAM E70 based MCU.

…ossed

Current implementation sends RX flow control activation every time the
limit is crossed when receive packet is read by driver. This may result
in repetitious sends of RX flow control signal when some packets are
left on the bus or in FIFO.

This change ensures RX flow control is send only when it is not active
(priv->iactive is false). It also removes break statement from while loop
when watermark is used. The driver should activate RX flow and read the
rest of the packet instead of discarding it.

Signed-off-by: Michal Lenc <[email protected]>
@michallenc michallenc marked this pull request as draft November 8, 2022 16:07
@michallenc
Copy link
Contributor Author

Moving to draft so it wont get merged accidentally before we agreed (or discard) this change.

@acassis
Copy link
Contributor

acassis commented Nov 8, 2022

@michallenc our modification seems correct to me. There is an example application (apps/example/flowc) that you can use to force the CDC/ACM device to its transfer limits and that could be used to confirm that everything is working as expected. Greg implemented it because a company was using the CDC/ACM to transfer data to their printer and it was losing data. After this implementation the issue was fixed.

@michallenc
Copy link
Contributor Author

@acassis Yes, the driver in current state is working as expected (tests with our application were also fine). The question is whether we should send the flow control status once or every time (both will work but not sure what is preferable).

@acassis
Copy link
Contributor

acassis commented Nov 9, 2022

@michallenc good question! The advantage of keep sending it is because if for some reason the other side missed the first one they could catch the next ones. Suggest: test on Linux with debug enabled to see how it is done on Linux. You will need a board that supports USB Gadgets subsystem (I think BBB and Raspberry Pi has OTG and support it)

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.

2 participants