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

Temp Fix for Delayed Buffering of Stop Response during Device Construction on Raspberry Pi #74

Merged
merged 5 commits into from
May 25, 2017

Conversation

dcyoung
Copy link

@dcyoung dcyoung commented Apr 28, 2017

This a temporary solution to the problems described in #73. See the issue for details.

It would be nice to know the root cause of the bug, but this temporary fix will allow development of the 3d-scanner to commence.

@dcyoung dcyoung requested a review from daniel-j-h April 28, 2017 17:43
@dcyoung dcyoung force-pushed the stop-scanning-buffer-fix branch from 4dc7161 to 6598b53 Compare April 29, 2017 00:36
// Wait some time for a few reasons:
// 1. allow time for the device to register the stop cmd and stop sending data blocks
// 2. allow time for slower performing devices (RaspberryPi) to buffer the stop response
// 3. allow a grace period for the device to perform its logging routine before sending a second stop command
Copy link
Collaborator

Choose a reason for hiding this comment

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

As said in the ticket, if we can just make the timeout below a bit longer I would go for that. Even hundreds of milliseconds are fine for construction if it makes things easier for us.

Copy link
Author

Choose a reason for hiding this comment

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

Check ticket for response.

@dcyoung dcyoung force-pushed the stop-scanning-buffer-fix branch 3 times, most recently from dbd50a2 to 6e9acab Compare May 8, 2017 20:27
@dcyoung
Copy link
Author

dcyoung commented May 8, 2017

Updated to adapt to the exceptions introduced by #80

@dcyoung
Copy link
Author

dcyoung commented May 11, 2017

Seems that this is definitely an issue other Raspberry Pi's as well, and that this fix is resolving the issue. See #88

@daniel-j-h Perhaps there is a better fix applicable to unix/serial.cc. I didn't write that code, but if you have any ideas for a possible source or fix let me know and I'd be happy to try and debug the unix/serial.cc.

In the meantime though, I'd like to get this merged as I think the Pi might be a common use case. Is there any reason we shouldn't merge this now, and bring a better fix in a later PR?

Edit: Appears this may be directly relevant: raspberrypi/linux#1709

It looks like a fix has been merged into the RPi firmware, only a few days ago. Hopefully that will make its way into the next raspbian release, which might solve the problem at its source. In the meantime, let's get this merged in yeah?

@daniel-j-h
Copy link
Collaborator

Urgh - great that it's fixed upstream. Feel free to merge if it's needed until Raspbian picks the fix up.

But please ticket this for later removal and maybe link it even from the source code. I don't want to add too many hacks for platform quirks.

There never should be a downside of doing this, right? Otherwise we need to detect the Raspberry env. with CMake and only conditionally enable the workaround.

@dcyoung dcyoung force-pushed the stop-scanning-buffer-fix branch 2 times, most recently from 5bcc611 to 5f069cd Compare May 17, 2017 22:47
@daniel-j-h
Copy link
Collaborator

@dcyoung still want to bring this in? I just added some release docs and a changelog here. Feel free to merge the release docs pull request first and then make a release with these Raspberry specific changes.

@dcyoung
Copy link
Author

dcyoung commented May 24, 2017

@daniel-j-h sounds good. I was dragging my feet a little, waiting to get a better estimate on the release timeframe for the Raspbian build.... but I think its silly to wait on it. The rPi will be a relatively common use case and I'd prefer to avoid confusion for those who aren't aware of this branch.

As for the release docs, I made some comments there. Let me know what you think, and if you want to keep things simple I'll merge them in that order. Additionally, should we merge #98 (API cleanup PR) later, or before all these others?

@daniel-j-h
Copy link
Collaborator

I think order does not really matter as there probably will be no conflicts.

@dcyoung dcyoung force-pushed the stop-scanning-buffer-fix branch from 5f069cd to 7dfecb7 Compare May 25, 2017 18:04
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.

2 participants