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

Fix msd extraneus bytes on reading #2245

Merged
merged 2 commits into from Sep 12, 2023

Conversation

maidnl
Copy link
Contributor

@maidnl maidnl commented Sep 6, 2023

Fix the fact that MSD status msg sometimes goes in the payload of data read from disk

This fixes: arduino/ArduinoCore-renesas#84
The problem: when issuing a reading command on MSD class, sometimes data read form disk (QSPI flash device) are corrupted with status message (the one that should "end" the transmission of data read from disk, i.e. "USBS..."). According to our analysis this is due to the fact that given that MSC_STAGE_STATUS case is executed always "after" the state machine implemented in mscd_xfer_cb function, if data are still present in the hw pipe fifo while the status message is sent by the function, the status message overwrites part of the data present in the hw pipe fifo.
The solution: a function that checks the status of the hw pipe fifo has been implemented. This is a weak function implemented only on RA hw. This function waits until the hw pipe fifo is empty. The function is then called in mscd_xfer_cb function when the status message is about to be sent while a reading from disk is finishing. This ensure that data in hw pipe fifo are not overwritten due to lack of sync.

If you have a better solution or a more general approach, please feel free to provide us with your implementation, we will test this on our device. Thanks!

@facchinm

@kkitayam
Copy link
Collaborator

kkitayam commented Sep 6, 2023

I guess that the following place is good to wait for the HW pipe FIFO to be empty. At the last packet of an IN transaction, it will wait for the HW pipe to be empty.

if data are still present in the hw pipe fifo while the status message is sent by the function, the status message overwrites part of the data present in the hw pipe fifo.

For efficient handling double buffer, I had designed pipe_xfer_in() returns true(It escalates to mscd_xfer_cb()) when there is at least one empty buffer in HW pipe FIFO. In case of bulk transfer, HW pipe is set double buffer mode, so pipe_xfer_in() will return true when one buffer is empty, even if there is data in the other buffer.
Therefore I guess the status message will not overwrite a part of previous data.

Any way, if we need to wait for HW pipe FIFO to be empty, I suggest the above place, to keep MSC driver API interface.

This fixes a problem found on MSD class where data read from from disks were sometimes partially overwritten by the status MSD message ("USBS...").
The function introduced wait for the hw fifo pipe to be empty, that prevent that new writing in the fife overwrite data which are not yet be transmitted by hw.
@facchinm facchinm force-pushed the fix_msd_extraneus_bytes_on_reading branch from e9def0a to 7ce4cfa Compare September 8, 2023 08:49
@maidnl
Copy link
Contributor Author

maidnl commented Sep 8, 2023

I made the changes you suggested and tested them on our device. Now the wait function is static in the RA file dcd_rusb2.c and used in the point you mentioned in the comment. The MSD result untouched in this way and all the changes are only for renesas.

In case you prefer some changes to this to better fit the overall architecture please let me know and I will test them on our device. Thanks.

@kkitayam
Copy link
Collaborator

kkitayam commented Sep 8, 2023

Thank you for changing and testing.

Copy link
Owner

@hathach hathach left a comment

Choose a reason for hiding this comment

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

superb! thank you @maidnl for the fix and @kkitayam for suggestion and reviewing.

@hathach hathach merged commit e9ba933 into hathach:master Sep 12, 2023
41 checks passed
@Rbb666
Copy link
Contributor

Rbb666 commented Mar 10, 2024

@kkitayam @maidnl Hello, I verified on the RA series MCU that when the msc and cdc devices are turned on, this modification will cause a usb enumeration time of about 20 seconds,and the operation of CDC and MSC is not stable.
image

@hathach
Copy link
Owner

hathach commented Apr 10, 2024

thanks @Rbb666, I just pull out my ram41 to test with, this PR indeed causes hanged (block wait) with msc_cdc example. This should be reverted, since the solution does not work as expected and seems to cause more serious issue.

@hathach hathach mentioned this pull request Apr 10, 2024
hathach added a commit that referenced this pull request Apr 10, 2024
@Rbb666
Copy link
Contributor

Rbb666 commented Apr 10, 2024

谢谢,我只是拿出我的 ram41 进行测试,这个 PR 确实会导致挂起(块等待),举个例子msc_cdc。这应该被还原,因为解决方案没有按预期工作,并且似乎会导致更严重的问题。

Yes, this is a problem I detected on Renesas ra8d1. I hope it can be fixed as soon as possible.

@hathach
Copy link
Owner

hathach commented Apr 10, 2024

already reverted by #2587

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.

QSPI bug with Portenta C33
4 participants