Get the correct value for the lastLineFlag from cnfLine #217
Conversation
WalkthroughThe changes in Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/ayab/com.cpp (2)
300-304: Consider adding validation for the flags byteWhile the fix is correct, consider adding validation to ensure the flags byte contains valid values, especially since this is a critical communication point between the Python host and the firmware.
// A Python bool is represented as a byte // so read the second bit to get the value + // Validate flags byte + if (flags & ~0x02) { // Check if any unexpected bits are set + // TODO: Consider how to handle unexpected flag values + // Options: log warning, send error response, or ignore + } bool flagLastLine = bitRead(flags, 1U);
Line range hint
293-297: Consider tracking existing TODOs as issuesThere are several TODOs in the code regarding error handling for CRC-8 checks and message length. While not directly related to this change, these represent important reliability concerns that should be tracked.
Would you like me to create GitHub issues to track these TODOs? I can help draft detailed descriptions for each:
- Handling CRC-8 checksum errors
- Handling message length validation
- Implementing error response for invalid messages
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/ayab/com.cpp(1 hunks)
🔇 Additional comments (1)
src/ayab/com.cpp (1)
301-303: LGTM! Correct fix for Python boolean representation
The change correctly addresses the issue of reading boolean values from Python by shifting from bit 0 to bit 1. The added comment provides clear documentation of the reasoning.
|
This patch can be tested by going to https://code.jonathanperret.net/ayab-webtool/#pr=217 . |
|
|
||
| // A Python bool is represented as a byte | ||
| // so read the second bit to get the value | ||
| bool flagLastLine = bitRead(flags, 1U); |
There was a problem hiding this comment.
Reading bit 1 and assuming the lastLine flag there is violating the API.
Assuming the error is really caused by a faulty Bool->int conversion in the Python (ayab-desktop) part of AYAB, it should be fixed there to comply with the API (likely https://github.com/AllYarnsAreBeautiful/ayab-desktop/blob/5e781f109c8c86f6190152cfa6db0095c888b242/src/main/python/main/ayab/engine/control.py#L212).
AllYarnsAreBeautiful/ayab-desktop#662
Python bools are bytes not bits
Summary by CodeRabbit
Bug Fixes
cnfLinecommand to correctly determine the last line flag.Chores