Fix GrovePi binding timing issue when read sensors - #2359
2427dkusiro wants to merge 4 commits into
Conversation
2427dkusiro
changed the title
dotnet-policy-service
Bot
added
the
area-device-bindings
|
@dotnet-policy-service agree |
| } | ||
| } | ||
|
|
||
| throw new IOException($"{nameof(DigitalRead)}: Failed to read byte with command {GrovePiCommand.DigitalRead}", innerEx); |
There was a problem hiding this comment.
Why are you removing the throw here and replace with the -1 logic? Throwing is a good way to say that things did not work properly.
There was a problem hiding this comment.
@Ellerbach
I initially considered making this change because the existing AnalogRead method was designed to return -1 instead of throwing an exception. However, I have adjusted the behavior at commit 93c6303 accordingly.
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
🟡 Changes recommended
DigitalRead currently returns an out-of-range PinValue (-1) on malformed responses and ReadCommand reports a misleading “Failed to write” error message, both of which should be corrected before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR addresses a Raspberry Pi 4 timing/response issue in the GrovePi binding by adding retry behavior when the device returns an “invalid / not ready” response, and by aligning DigitalRead with the common read path used by other sensor reads.
Changes:
- Increased the retry budget for I2C operations (
MaxRetriesfrom 4 to 10). - Updated
ReadCommandto retry when the response indicates “data not available” (0x17 / 23) or an invalid response (0xFF / 255). - Refactored
DigitalReadto useReadCommand(and adjusted expected response length).
File summaries
| File | Description |
|---|---|
| src/devices/GrovePi/GrovePi.cs | Adds response-validation retries in ReadCommand, increases retry count, and routes DigitalRead through the shared read path. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| var data = ReadCommand(GrovePiCommand.DigitalRead, pin); | ||
| if (data is null || data.Length < 2) | ||
| { | ||
| try | ||
| { | ||
| return (PinValue)_i2cDevice.ReadByte(); | ||
| } | ||
| catch (IOException ex) | ||
| { | ||
| // Give it another try | ||
| innerEx = ex; | ||
| tries++; | ||
| Thread.Sleep(10); | ||
| } | ||
| return (PinValue)(-1); | ||
| } |
| /// <returns></returns> | ||
| public byte[]? ReadCommand(GrovePiCommand command, GrovePort pin) | ||
| { | ||
| const int dataNotAvailableCommand = 23; |
| if (outArray[0] != dataNotAvailableCommand && outArray[0] != 255) | ||
| { | ||
| return outArray; | ||
| } |
| } | ||
|
|
||
| throw new IOException($"{nameof(ReadCommand)}: Failed to write command {command}", innerEx); |
Fixes #2337
Summary
Fix timing issue in the GrovePi Binding logic for sensor reading.
Detail
As mentioned in the issue I raised, there is a bug in the current GrovePi Binding that prevents it from correctly reading sensor values on the Raspberry Pi 4. To fix this bug, I have modified the code to wait and retry when an invalid response is received from the device. Additionally, I've standardized the implementation of the
DigitalReadmethod with other sensor readings so that it can also benefit from this bug fix.Validation
I have confirmed that the changes in this PR work as intended by testing with sample code from the GrovePi directory. I connected a button, potentiometer, relay, ultrasonic distance sensor, and temperature & humidity sensor to both Raspberry Pi 3 and 4.
Microsoft Reviewers: Open in CodeFlow