Skip to content

Resolve a set of critical bugs in SERCOM and unify SPI/UART/I2C baudrate calculations #381

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

Open
crabel99 wants to merge 4 commits into
base: master
Choose a base branch
from
+90 −52
Open

Resolve a set of critical bugs in SERCOM and unify SPI/UART/I2C baudrate calculations #381

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e0a883c
Unify SERCOM clock frequency handling across UART/SPI/WIRE
crabel99 Jan 13, 2026
d2772f8
bug: resolve low speed I2C BAUD register overflow
crabel99 Jan 13, 2026
eebc1ea
avoid float division
hathach Jan 20, 2026
7941541
avoid float division 2
hathach Jan 23, 2026
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
121 changes: 79 additions & 42 deletions cores/arduino/SERCOM.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ SERCOM::SERCOM(Sercom* s)
{
sercom = s;

#if defined(__SAMD51__)
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
// A briefly-available but now deprecated feature had the SPI clock source
// set via a compile-time setting (MAX_SPI)...problem was this affected
// ALL SERCOMs, whereas some (anything read/write, e.g. SD cards) should
Expand All @@ -40,8 +40,8 @@ SERCOM::SERCOM(Sercom* s)
// per-peripheral basis. Nonetheless, we check SERCOM_SPI_FREQ_REF here
// (MAX_SPI * 2) to retain compatibility with any interim projects that
// might have relied on the compile-time setting. But please, don't.
#if SERCOM_SPI_FREQ_REF == F_CPU // F_CPU clock = GCLK0
clockSource = SERCOM_CLOCK_SOURCE_FCPU;
#if SERCOM_SPI_FREQ_REF == F_CPU // F_CPU clock = GCLK0
clockSource = SERCOM_CLOCK_SOURCE_100M;
#elif SERCOM_SPI_FREQ_REF == 48000000 // 48 MHz clock = GCLK1 (standard)
clockSource = SERCOM_CLOCK_SOURCE_48M;
#elif SERCOM_SPI_FREQ_REF == 100000000 // 100 MHz clock = GCLK2
Expand Down Expand Up @@ -80,11 +80,7 @@ void SERCOM::initUART(SercomUartMode mode, SercomUartSampleRate sampleRate, uint
// Asynchronous fractional mode (Table 24-2 in datasheet)
// BAUD = fref / (sampleRateValue * fbaud)
// (multiply by 8, to calculate fractional piece)
#if defined(__SAMD51__)
uint32_t baudTimes8 = (SERCOM_FREQ_REF * 8) / (sampleRateValue * baudrate);
#else
uint32_t baudTimes8 = (SystemCoreClock * 8) / (sampleRateValue * baudrate);
#endif
uint32_t baudTimes8 = (freqRef * 8) / (sampleRateValue * baudrate);

sercom->USART.BAUD.FRAC.FP = (baudTimes8 % 8);
sercom->USART.BAUD.FRAC.BAUD = (baudTimes8 / 8);
Expand Down Expand Up @@ -230,8 +226,8 @@ void SERCOM::initSPI(SercomSpiTXPad mosi, SercomRXPad miso, SercomSpiCharSize ch
resetSPI();
initClockNVIC();

#if defined(__SAMD51__)
sercom->SPI.CTRLA.reg = SERCOM_SPI_CTRLA_MODE(0x3) | // master mode
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
sercom->SPI.CTRLA.reg = SERCOM_SPI_CTRLA_MODE(0x3) | // master mode
SERCOM_SPI_CTRLA_DOPO(mosi) |
SERCOM_SPI_CTRLA_DIPO(miso) |
dataOrder << SERCOM_SPI_CTRLA_DORD_Pos;
Expand Down Expand Up @@ -322,13 +318,7 @@ SercomDataOrder SERCOM::getDataOrderSPI()
void SERCOM::setBaudrateSPI(uint8_t divider)
{
disableSPI(); // Register is enable-protected

#if defined(__SAMD51__)
sercom->SPI.BAUD.reg = calculateBaudrateSynchronous(freqRef / divider);
#else
sercom->SPI.BAUD.reg = calculateBaudrateSynchronous(SERCOM_SPI_FREQ_REF / divider);
#endif

enableSPI();
}

Expand Down Expand Up @@ -386,13 +376,11 @@ bool SERCOM::isDataRegisterEmptySPI()
// return sercom->SPI.INTFLAG.bit.RXC;
//}

uint8_t SERCOM::calculateBaudrateSynchronous(uint32_t baudrate) {
#if defined(__SAMD51__)
uint8_t SERCOM::calculateBaudrateSynchronous(uint32_t baudrate)
{
uint16_t b = freqRef / (2 * baudrate);
#else
uint16_t b = SERCOM_SPI_FREQ_REF / (2 * baudrate);
#endif
if(b > 0) b--; // Don't -1 on baud calc if already at 0
if (b > 0)
b--; // Don't -1 on baud calc if already at 0
return b;
}

Expand Down Expand Up @@ -490,12 +478,17 @@ void SERCOM::initMasterWIRE( uint32_t baudrate )
// Enable all interrupts
// sercom->I2CM.INTENSET.reg = SERCOM_I2CM_INTENSET_MB | SERCOM_I2CM_INTENSET_SB | SERCOM_I2CM_INTENSET_ERROR ;

// Synchronous arithmetic baudrate
#if defined(__SAMD51__)
sercom->I2CM.BAUD.bit.BAUD = SERCOM_FREQ_REF / ( 2 * baudrate) - 1 ;
#else
sercom->I2CM.BAUD.bit.BAUD = SystemCoreClock / ( 2 * baudrate) - 5 - (((SystemCoreClock / 1000000) * WIRE_RISE_TIME_NANOSECONDS) / (2 * 1000));
#endif
uint8_t speedBit = sercom->I2CM.CTRLA.bit.SPEED;
Copy link

Copilot AI Jan 19, 2026

Choose a reason for hiding this comment

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

The SPEED bits are being read at line 481 before they are set. The CTRLA register is set at line 471 but doesn't set the SPEED field. This means speedBit will be 0 (the reset default) and not the intended speed mode, which could lead to incorrect I2C speed configuration and baudrate calculation. The SPEED bits should be set in CTRLA before reading them, or the intended speed mode should be passed as a parameter to initMasterWIRE.

Copilot uses AI. Check for mistakes.
Copy link
Author

@crabel99 crabel99 Jan 19, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Copilot is correct about the placement of setting speedBit. It needs to be done before it is used. This comment brings up two additional issues:

1. Dynamic setting of SPEED

The initMasterWIRE(baudrate) API sets the BAUD register based on how the registers are configured and baudrate. The setting of the two SPEED bits is a different programatic change in the API than what this bug fix is. I wanted to keep the code constrained.

If you wanted to change the API's function and dynamically assign the SPEED based on baudrate, replace int8_t speedBit = sercom->I2CM.CTRLA.bit.SPEED; with:

 // Determine speed mode based on requested baudrate
  uint32_t topSpeeds[3] = {400000, 1000000, 3400000}; // {(sm/fm), (fm+), (hs)}
  uint8_t speedBit = 0;
  bool clockStretchMode = false;
  sercom->I2CM.CTRLA.bit.SCLSM = 0; // See: 28.6.2.4.6

  if (baudrate > topSpeeds[0] && baudrate <= topSpeeds[1])
    {
    speedBit = 1; // Fast mode+
    clockStretchMode = false; // See: 28.6.2.4.6
    }
  else if (baudrate > topSpeeds[1])
    {
    speedBit = 2; // High speed
    clockStretchMode = true; // See: 28.6.2.4.6
    }
  // else speedBit = 0 and clockStretchMode = false for Standard/Fast mode (up to 400kHz)
  
  sercom->I2CM.CTRLA.bit.SPEED = speedBit;
  sercom->I2CM.CTRLA.bit.SCLSM = clockStretchMode; // See: 28.6.2.4.6

But, you need to be careful as (Hs-mode) requires very specific register configurations that need to be set.

  • Requires an API to configure I2CS.CTRLA.bit.SPEED I2CS.CTRLA.bit.SCLSM for High-speed mode Client operations 28.6.2.5 and 28.6.2.5.4.
  • I have not reviewed how the transfers for both read and write in Host/Client are done, nor tested these in High-speed or even Fast-mode +. These changes correctly activate full dynamic control of all transfer rates, but whether they work or not is another issue.
  • I2CM.ADDR.bit.HS needs to be set to enable High-speed transfers in Host mode 28.6.2.4.6 and 28.10.9. This would need to be done when the address is written to ADDR. See:
bool SERCOM::startTransmissionWIRE(uint8_t address, SercomWireReadWriteFlag flag)
{
  ...
  sercom->I2CM.ADDR.reg |= address | ((sercom->I2CM.CTRLA.bit.SPEED == 0x2) << 14);
  ...
}

2. Error in the PR variable specification

The snippet Copilot provided should be freqRef not SystemCoreClock:

  if (speedBit == 0x2)
    sercom->I2CM.BAUD.bit.HSBAUD = freqRef / (2 * baudrate) - 1;
  else
    sercom->I2CM.BAUD.bit.BAUD = freqRef / (2 * baudrate) - 5 - freqRef * static_cast<uint16_t>(WIRE_RISE_TIME_NANOSECONDS) / (2 * 1e9f);

Edits

  • Added comment about the status of the implementation of dynamic SPEED control.

Copy link
Member

@hathach hathach Jan 20, 2026

Choose a reason for hiding this comment

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

I think we should just keep the API and use baudrate to also set the SPEED bit accordingly to the topspeed e.g < 400khz, speed=0, <1Mhz speed =1, otherwise speed =2. and also configure other register if neccessary. Would mind updating the PR to do so if possible. Don't worry about doing actual speed test for this function, since there maybe other factor as well.

const uint32_t topSpeeds[3] = {400000, 1000000, 3400000}; // {(sm/fm), (fm+), (hs)}
const uint32_t minBaudrate = freqRef / 512; // BAUD = 255: SAMD51 ~195kHz, SAMD21 ~94kHz
const uint32_t maxBaudrate = topSpeeds[speedBit];
baudrate = max(minBaudrate, min(baudrate, maxBaudrate));

if (speedBit == 0x2)
sercom->I2CM.BAUD.bit.HSBAUD = freqRef / (2 * baudrate) - 1;
else
sercom->I2CM.BAUD.bit.BAUD = freqRef / (2 * baudrate) - 5 -
(freqRef/1000000ul * WIRE_RISE_TIME_NANOSECONDS) / 2000;
}

void SERCOM::prepareNackBitWIRE( void )
Expand Down Expand Up @@ -727,7 +720,7 @@ uint8_t SERCOM::readDataWIRE( void )
}
}

#if defined(__SAMD51__)
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)

static const struct {
Sercom *sercomPtr;
Expand Down Expand Up @@ -785,7 +778,52 @@ int8_t SERCOM::getSercomIndex(void) {
return -1;
}

#if defined(__SAMD51__)
uint32_t SERCOM::getSercomFreqRef(void)
{
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
int8_t idx = getSercomIndex();
uint8_t gen = 1; // default to GCLK1 (48 MHz) if we can't resolve

if (idx >= 0)
{
uint8_t pch = sercomData[idx].id_core;
gen = GCLK->PCHCTRL[pch].bit.GEN;
}

// GCLK0 = F_CPU
// GCLK1 = 48 MHz
// GCLK2 = 100 MHz
// GCLK3 = XOSC32K
// GCLK4 = 12 MHz
switch (gen)
{
case 0:
freqRef = 100000000UL; // F_CPU but limit at 100 Mhz for SERCOM ref clock
break;
case 1:
freqRef = 48000000UL;
break;
case 2:
freqRef = 100000000UL;
break;
case 3:
freqRef = 32768UL;
break;
case 4:
freqRef = 12000000UL;
break;
default:
freqRef = 48000000UL;
break;
}
#else
freqRef = SystemCoreClock;
#endif

return freqRef;
}

#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
// This is currently for overriding an SPI SERCOM's clock source only --
// NOT for UART or WIRE SERCOMs, where it will have unintended consequences.
// It does not check.
Expand All @@ -804,16 +842,19 @@ void SERCOM::setClockSource(int8_t idx, SercomClockSource src, bool core) {
if(core) clockSource = src; // Save SercomClockSource value

// From cores/arduino/startup.c:
// GCLK0 = F_CPU
// GCLK0 = F_CPU (this is 120 MHz and exceeds SERCOM maximum, limit to 100Mhz)
// GCLK1 = 48 MHz
// GCLK2 = 100 MHz
// GCLK3 = XOSC32K
// GCLK4 = 12 MHz
if(src == SERCOM_CLOCK_SOURCE_FCPU) {
GCLK->PCHCTRL[clk_id].reg =
GCLK_PCHCTRL_GEN_GCLK0_Val | (1 << GCLK_PCHCTRL_CHEN_Pos);
if(core) freqRef = F_CPU; // Save clock frequency value
} else if(src == SERCOM_CLOCK_SOURCE_48M) {
GCLK_PCHCTRL_GEN_GCLK2_Val | (1 << GCLK_PCHCTRL_CHEN_Pos); // Guard Sercom from exceeding 100 MHz maximum
if (core)
freqRef = 100000000; // Save clock frequency value
}
else if (src == SERCOM_CLOCK_SOURCE_48M)
{
GCLK->PCHCTRL[clk_id].reg =
GCLK_PCHCTRL_GEN_GCLK1_Val | (1 << GCLK_PCHCTRL_CHEN_Pos);
if(core) freqRef = 48000000;
Expand All @@ -840,21 +881,15 @@ void SERCOM::initClockNVIC( void )
int8_t idx = getSercomIndex();
if(idx < 0) return; // We got a problem here

#if defined(__SAMD51__)
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)

for(uint8_t i=0; i<4; i++) {
NVIC_ClearPendingIRQ(sercomData[idx].irq[i]);
NVIC_SetPriority(sercomData[idx].irq[i], SERCOM_NVIC_PRIORITY);
NVIC_EnableIRQ(sercomData[idx].irq[i]);
}

// SPI DMA speed is dictated by the "slow clock" (I think...maybe) so
// BOTH are set to the same clock source (clk_slow isn't sourced from
// XOSC32K as in prior versions of SAMD core).
// This might have power implications for sleep code.

setClockSource(idx, clockSource, true); // true = core clock
setClockSource(idx, clockSource, false); // false = slow clock
setClockSource(idx, clockSource, true); // true = core clock

#else // end if SAMD51 (prob SAMD21)

Expand All @@ -875,4 +910,6 @@ void SERCOM::initClockNVIC( void )
while(GCLK->STATUS.reg & GCLK_STATUS_SYNCBUSY); // Wait for synchronization

#endif // end !SAMD51

getSercomFreqRef();
}
21 changes: 11 additions & 10 deletions cores/arduino/SERCOM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,10 +214,10 @@ class SERCOM

void resetWIRE( void ) ;
void enableWIRE( void ) ;
void disableWIRE( void );
void prepareNackBitWIRE( void ) ;
void prepareAckBitWIRE( void ) ;
void prepareCommandBitsWire(uint8_t cmd);
void disableWIRE( void );
void prepareNackBitWIRE( void ) ;
void prepareAckBitWIRE( void ) ;
void prepareCommandBitsWire(uint8_t cmd);
bool startTransmissionWIRE(uint8_t address, SercomWireReadWriteFlag flag) ;
bool sendDataMasterWIRE(uint8_t data) ;
bool sendDataSlaveWIRE(uint8_t data) ;
Expand All @@ -233,11 +233,12 @@ class SERCOM
bool isRestartDetectedWIRE( void ) ;
bool isAddressMatch( void ) ;
bool isMasterReadOperationWIRE( void ) ;
bool isRXNackReceivedWIRE( void ) ;
bool isRXNackReceivedWIRE( void ) ;
int availableWIRE( void ) ;
uint8_t readDataWIRE( void ) ;
int8_t getSercomIndex(void);
#if defined(__SAMD51__)
uint32_t getSercomFreqRef(void);
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
// SERCOM clock source override is only available on
// SAMD51 (not 21) ... but these functions are declared
// regardless so user code doesn't need ifdefs or lengthy
Expand All @@ -253,11 +254,11 @@ class SERCOM
uint32_t getFreqRef(void) { return F_CPU; };
#endif

private:
Sercom* sercom;
#if defined(__SAMD51__)
private:
Sercom *sercom;
uint32_t freqRef = 48000000ul; // Frequency corresponding to clockSource
#if defined(__SAMD51__) || defined(__SAME51__) || defined(__SAME53__) || defined(__SAME54__)
SercomClockSource clockSource;
uint32_t freqRef; // Frequency corresponding to clockSource
#endif
uint8_t calculateBaudrateSynchronous(uint32_t baudrate);
uint32_t division(uint32_t dividend, uint32_t divisor) ;
Expand Down