Sercom: partially revert error handling change #378
Conversation
Revert a part of ccfc7db. It breaks I2C communcation.
There was a problem hiding this comment.
Pull request overview
This PR partially reverts a previous commit (ccfc7db) that broke I2C communication by changing error handling in the sendDataMasterWIRE function. The change reverts from checking the general INTFLAG.bit.ERROR flag back to checking only the specific STATUS.bit.BUSERR flag.
Key Changes:
- Reverted error detection in
sendDataMasterWIREfromINTFLAG.bit.ERRORtoSTATUS.bit.BUSERRto fix I2C communication issues reported in issue #376
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // The data transfer errors that can occur (including BUSERR) are all | ||
| // rolled up into INTFLAG.bit.ERROR from STATUS.reg | ||
| if (sercom->I2CM.INTFLAG.bit.ERROR) { | ||
| if (sercom->I2CM.STATUS.bit.BUSERR) { |
There was a problem hiding this comment.
This change creates an inconsistency in error handling. Other similar I2C wait loops in this file check INTFLAG.bit.ERROR (lines 565, 578, 716), which according to the comment on line 563-564 includes all data transfer errors "rolled up from STATUS.reg".
Checking only STATUS.bit.BUSERR here may miss other error conditions like arbitration loss (ARBLOST), bus errors, etc. While this may fix the immediate I2C communication issue mentioned in #376, consider whether:
- The same change should be applied to the other similar wait loops for consistency
- Or if there's a different underlying issue that should be addressed
The comment on lines 606-607 also says "data transfer error" (plural) but only one error type is now being checked.
| if (sercom->I2CM.STATUS.bit.BUSERR) { | |
| if (sercom->I2CM.INTFLAG.bit.ERROR) { |
| // If a data transfer error occurs, the MB bit may never be set. | ||
| // Check the error bit and bail if it's set. |
There was a problem hiding this comment.
The comment says "If a data transfer error occurs" (suggesting multiple error types), but the code now only checks for BUSERR. Consider updating the comment to be more specific, e.g., "If a bus error occurs" or explain why only BUSERR is checked here while other similar functions check all errors via INTFLAG.bit.ERROR.
2 participants
Revert a part of ccfc7db. It breaks I2C communcation.
#376