Skip to content

Sercom: partially revert error handling change #378

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.

Sign up for GitHub

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

Merged
hathach merged 1 commit into from
Dec 5, 2025
Merged

Sercom: partially revert error handling change #378

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions cores/arduino/SERCOM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -605,9 +605,7 @@ bool SERCOM::sendDataMasterWIRE(uint8_t data)
while(!sercom->I2CM.INTFLAG.bit.MB) {
// If a data transfer error occurs, the MB bit may never be set.
// Check the error bit and bail if it's set.
Comment on lines 606 to 607
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
// 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) {
Copy link

Copilot AI Dec 5, 2025

Choose a reason for hiding this comment

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

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:

  1. The same change should be applied to the other similar wait loops for consistency
  2. 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.

Suggested change
if (sercom->I2CM.STATUS.bit.BUSERR) {
if (sercom->I2CM.INTFLAG.bit.ERROR) {

Copilot uses AI. Check for mistakes.
return false;
}
}
Expand Down