Conversation
pic18f26k83/i2c.c
Outdated
|
|
||
| bool i2c_read_reg8(uint8_t address, uint8_t reg, uint8_t *value) { | ||
| return i2c_read_data(address, reg, value, 1); | ||
| bool success = i2c_write(address, ®, 1); |
There was a problem hiding this comment.
Add some comments with a link to the data sheet of a relevent IC explaining what this protocol is and why its needed
JasonBrave
force-pushed
the
i2c-compile-fix
branch
from
7eebde5 to
9fe4601
Compare
JasonBrave
force-pushed
the
i2c-compile-fix
branch
from
9fe4601 to
41cc05c
Compare
taisirhassan
force-pushed
the
i2c-compile-fix
branch
from
41cc05c to
da5840b
Compare
taisirhassan
force-pushed
the
i2c-compile-fix
branch
from
a10863c to
11a2c7c
Compare
taisirhassan
force-pushed
the
i2c-compile-fix
branch
from
11a2c7c to
0f5e93c
Compare
…ng and type safety
…improve internal state checks for better error handling
…error handling and clarity
taisirhassan
force-pushed
the
i2c-compile-fix
branch
from
f8334ae to
b997794
Compare
… handling, add bus recovery procedure, and update function signatures for consistency.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
WalkthroughThe I2C driver API is standardized to return Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
pic18f26k83/i2c.c (1)
288-360: Extract shared register-read transaction logic to one helper.
i2c_read_reg8andi2c_read_reg16duplicate nearly the same state machine, which increases drift risk and bug surface.♻️ Refactor direction
+static w_status_t i2c_read_reg_common(uint8_t address, uint8_t reg, uint8_t *buf, uint8_t nbytes); + w_status_t i2c_read_reg8(uint8_t address, uint8_t reg, uint8_t *value) { - /* duplicated transfer flow */ + return i2c_read_reg_common(address, reg, value, 1U); } w_status_t i2c_read_reg16(uint8_t address, uint8_t reg, uint16_t *value) { - /* duplicated transfer flow */ + uint8_t tmp[2]; + w_status_t s = i2c_read_reg_common(address, reg, tmp, 2U); + if (s != W_SUCCESS) return s; + *value = ((uint16_t)tmp[0] << 8) | tmp[1]; + return W_SUCCESS; }Also applies to: 363-450
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pic18f26k83/i2c.c` around lines 288 - 360, i2c_read_reg8 and i2c_read_reg16 duplicate the same I2C state machine; extract the shared transaction into a helper (e.g., i2c_read_register_transaction) that accepts parameters: uint8_t address, const uint8_t *reg_bytes, size_t reg_len, uint8_t *out_buf, size_t out_len, and performs the common sequence (call check_i2c_state, clear_i2c_buffers, set I2C1ADB1/I2C1CNT for write, wait loops checking I2C1STAT1bits.TXBE / I2C1PIRbits.PCIF and I2C1ERRbits.NACKIF/I2C1ERRbits.BCLIF with I2C_POLL_TIMEOUT, send register bytes, issue repeated START with (address<<1)|1, configure I2C1CON1bits.ACKDT/ACKCNT/ I2C1CON0bits.RSEN, wait for RXBF, copy I2C1RXB(s) into out_buf, wait for PCIF), return w_status_t; then simplify i2c_read_reg8 and i2c_read_reg16 to call this helper with a 1- or 2-byte reg and appropriate out_len. Ensure the helper clears/reads I2C1ERR and preserves the same error returns (W_IO_ERROR/W_IO_TIMEOUT/W_SUCCESS).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@doc/i2c_driver.rst`:
- Around line 15-16: Fix the grammar and register naming in the doc: change
"Register address are 8 bit wide" to "Register addresses are 8-bit wide" and
replace the reference to I2CCNT with the actual symbol used in the
implementation, I2C1CNT (e.g., "Automatic STOP generation when I2C1CNT reaches 0
(PIC18F26K83 standalone I2C module)"). Ensure the wording and register names
match the codebase.
In `@pic18f26k83/i2c.c`:
- Around line 154-159: Guard pointer parameters before any dereference: in
i2c_write check that 'data' is not NULL before using it and return an
appropriate w_status_t (e.g., W_ERR_INVALID_PARAM) after check_i2c_state; apply
the same pattern to the other I2C helpers referenced (e.g., i2c_read — validate
'data' buffer, i2c_write_byte and i2c_read_byte — validate any 'value'
pointer/argument if it is dereferenced) so callers passing NULL don't cause
crashes. Locate these checks in the functions named i2c_write, i2c_read,
i2c_write_byte, i2c_read_byte and add early returns on invalid pointer inputs
with a clear error status.
- Around line 183-187: The loop resets timeout whenever I2C1CON0bits.CSTR is
set, which allows a permanently stretched clock to prevent reaching
I2C_POLL_TIMEOUT and hang; change the logic in the polling loops (the block
checking I2C1CON0bits.CSTR, the timeout variable, I2C_STRETCH_DELAY and the
I2C_POLL_TIMEOUT comparison that returns W_IO_TIMEOUT) to stop resetting timeout
while CSTR is asserted — instead delay when CSTR is set but continue to let
timeout advance (or increment a dedicated elapsed counter while delaying) so the
timeout check can eventually return W_IO_TIMEOUT; apply the same fix to the
repeated block around the other occurrence (the one referenced similarly at
lines ~240-244).
- Around line 121-127: The clkdiv argument passed to i2c_init must be explicitly
validated (allowed range 0–7) before programming CLKRCON to avoid masking caller
bugs; update the i2c_init function to check if clkdiv > 7 and return an
appropriate error w_status_t (e.g., failure/invalid-parameter) prior to writing
CLKRCON/CLKRCLK and disabling EN, so only validated values are ever used when
setting CLKRCON.
- Around line 220-224: The ACKCNT field must be explicitly set to 1 for every
master read to ensure the final byte is NACKed; change the read transaction code
that currently conditionally sets I2C1CON1bits.ACKCNT based on len (the block
around I2C1CON1bits.ACKDT and the if (len == 1) { I2C1CON1bits.ACKCNT = 1; } )
so that I2C1CON1bits.ACKCNT = 1 is assigned unconditionally for all receive
operations (use the same location where ACKDT is configured).
- Around line 79-87: The bus-recovery code currently drives SCL/SDA via LATC
(LATCbits.LATC3/LATCbits.LATC4) which violates I2C open-drain semantics; change
recovery to use TRIS lines so "release high" = set TRISCbits.TRISCx = 1 and
"pull low" = set TRISCbits.TRISCx = 0 with LATCbits.LATCx = 0 preloaded for low,
and update the I2C initialization to enable open-drain on the pins by setting
ODCONC3 and ODCONC4; apply this pattern in the recovery loop (the for-loop that
pulses SCL) and where SDA/SCL are toggled later so lines are released rather
than actively driven high.
---
Nitpick comments:
In `@pic18f26k83/i2c.c`:
- Around line 288-360: i2c_read_reg8 and i2c_read_reg16 duplicate the same I2C
state machine; extract the shared transaction into a helper (e.g.,
i2c_read_register_transaction) that accepts parameters: uint8_t address, const
uint8_t *reg_bytes, size_t reg_len, uint8_t *out_buf, size_t out_len, and
performs the common sequence (call check_i2c_state, clear_i2c_buffers, set
I2C1ADB1/I2C1CNT for write, wait loops checking I2C1STAT1bits.TXBE /
I2C1PIRbits.PCIF and I2C1ERRbits.NACKIF/I2C1ERRbits.BCLIF with I2C_POLL_TIMEOUT,
send register bytes, issue repeated START with (address<<1)|1, configure
I2C1CON1bits.ACKDT/ACKCNT/ I2C1CON0bits.RSEN, wait for RXBF, copy I2C1RXB(s)
into out_buf, wait for PCIF), return w_status_t; then simplify i2c_read_reg8 and
i2c_read_reg16 to call this helper with a 1- or 2-byte reg and appropriate
out_len. Ensure the helper clears/reads I2C1ERR and preserves the same error
returns (W_IO_ERROR/W_IO_TIMEOUT/W_SUCCESS).
| - Register address are 8 bit wide | ||
| - Automatic STOP generation when I2CCNT reaches 0 (PIC18F26K83 standalone I2C module) |
There was a problem hiding this comment.
Tighten assumption wording and register naming.
Line 15 grammar is off, and Line 16 uses I2CCNT while the implementation uses I2C1CNT. Please align to reduce ambiguity.
✏️ Proposed doc fix
-- Register address are 8 bit wide
-- Automatic STOP generation when I2CCNT reaches 0 (PIC18F26K83 standalone I2C module)
+- Register addresses are 8-bit wide
+- Automatic STOP generation when I2C1CNT reaches 0 (PIC18F26K83 standalone I2C module)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@doc/i2c_driver.rst` around lines 15 - 16, Fix the grammar and register naming
in the doc: change "Register address are 8 bit wide" to "Register addresses are
8-bit wide" and replace the reference to I2CCNT with the actual symbol used in
the implementation, I2C1CNT (e.g., "Automatic STOP generation when I2C1CNT
reaches 0 (PIC18F26K83 standalone I2C module)"). Ensure the wording and register
names match the codebase.
| TRISCbits.TRISC3 = 0; // SCL as output | ||
| TRISCbits.TRISC4 = 0; // SDA as output | ||
|
|
||
| // Try to unstick SDA with up to 9 clock pulses | ||
| for (uint8_t i = 0; i < 9; i++) { | ||
| LATCbits.LATC3 = 0; // SCL low | ||
| __delay_us(5); | ||
| LATCbits.LATC3 = 1; // SCL high | ||
| __delay_us(5); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify RC3/RC4 open-drain and I2C configuration
cd /tmp || exit 1
rg -n -C2 'ODCON|ODCONC|TRISC3|TRISC4|RC3PPS|RC4PPS|I2C1SCL|I2C1SDA|ANSELC|WPUC' pic18f26k83/i2c.cRepository: waterloo-rocketry/rocketlib
Length of output: 128
🏁 Script executed:
#!/bin/bash
# Find and read the i2c.c file to understand the context
fd -n i2c.c | head -5Repository: waterloo-rocketry/rocketlib
Length of output: 301
🏁 Script executed:
#!/bin/bash
# Check file structure and get relevant lines
if [ -f "pic18f26k83/i2c.c" ]; then
wc -l pic18f26k83/i2c.c
echo "=== Lines 70-120 ==="
sed -n '70,120p' pic18f26k83/i2c.c
fiRepository: waterloo-rocketry/rocketlib
Length of output: 1475
🏁 Script executed:
#!/bin/bash
# Search for ODCON, ANSELC, PPS, or related I2C pin configurations
rg 'ODCON|ODCONC|ANSELC|WPUC|RC3PPS|RC4PPS|I2C1SCL|I2C1SDA' --type cRepository: waterloo-rocketry/rocketlib
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Look at the entire bus_recovery function and surrounding context
sed -n '65,115p' pic18f26k83/i2c.cRepository: waterloo-rocketry/rocketlib
Length of output: 1544
🏁 Script executed:
#!/bin/bash
# Check PIC18F26K83 datasheet context - search for any setup_pins or initialization functions
rg -A 10 'void.*init|setup' pic18f26k83/i2c.c | head -50Repository: waterloo-rocketry/rocketlib
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Search for any ODCON configuration or open-drain setup
rg -i 'odcon|open.drain' --type cRepository: waterloo-rocketry/rocketlib
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Check if there's any pin configuration file or header
fd -e h -e c | xargs grep -l 'TRISC\|ODCON\|pin.*config' | head -10Repository: waterloo-rocketry/rocketlib
Length of output: 120
🏁 Script executed:
#!/bin/bash
# Look for the complete context - check lines around the second occurrence (101-104)
sed -n '95,110p' pic18f26k83/i2c.cRepository: waterloo-rocketry/rocketlib
Length of output: 515
🏁 Script executed:
#!/bin/bash
# Check the entire i2c.c file for any ODCON or open-drain related code
cat pic18f26k83/i2c.c | grep -n -i 'odcon\|open.drain\|drain\|LATCbits\|TRISCbits'Repository: waterloo-rocketry/rocketlib
Length of output: 574
🏁 Script executed:
#!/bin/bash
# Check for any I2C module initialization that might configure open-drain
sed -n '1,70p' pic18f26k83/i2c.cRepository: waterloo-rocketry/rocketlib
Length of output: 1901
🏁 Script executed:
#!/bin/bash
# Look for WPUC (weak pull-up), pull-up configuration, or I2C module init
sed -n '1,150p' pic18f26k83/i2c.c | grep -n -i 'wpuc\|pull\|init.*i2c\|I2C1CON'Repository: waterloo-rocketry/rocketlib
Length of output: 339
🏁 Script executed:
#!/bin/bash
# Check if there's a separate init function or header with pin config
cat pic18f26k83/i2c.h 2>/dev/null || echo "Header not found, checking for any config mentions"Repository: waterloo-rocketry/rocketlib
Length of output: 121
🏁 Script executed:
#!/bin/bash
# Search entire codebase for I2C initialization or pin setup
rg -n 'I2C1CON\|I2C1*' pic18f26k83/i2c.c | head -20Repository: waterloo-rocketry/rocketlib
Length of output: 53
🏁 Script executed:
#!/bin/bash
# Get complete I2C initialization function
sed -n '130,160p' pic18f26k83/i2c.cRepository: waterloo-rocketry/rocketlib
Length of output: 771
🏁 Script executed:
#!/bin/bash
# Search entire repo for ODCON or any configuration that might enable open-drain
rg -r 'ODCON' . 2>/dev/null || echo "No ODCON found"Repository: waterloo-rocketry/rocketlib
Length of output: 50384
🏁 Script executed:
#!/bin/bash
# Check PIC18F26K83 specific - look for any port configuration
rg -i 'ansela|anselb|anselc|anseld|ansele|trisb|trisb' pic18f26k83/ --type cRepository: waterloo-rocketry/rocketlib
Length of output: 53
🌐 Web query:
PIC18F26K83 I2C open-drain pin configuration RC3 RC4
💡 Result:
On the PIC18F26K83, if you use RC3 = SCL1 and RC4 = SDA1 (the device’s dedicated I²C-capable pads), configure them like this:
- Make RC3/RC4 digital pins (disable analog on those bits).
- Set the pins as inputs (the I²C peripheral manipulates the lines when TRIS=1, and the bus is wired-AND).
- Enable open-drain on both pins: set ODCC3=1 and ODCC4=1 in ODCONC (Microchip explicitly requires ODCON for I²C on this family). [1], [2]
- Route MSSP/I2C1 to RC3/RC4 using PPS: map both the input PPS registers (I2C1SCLPPS/I2C1SDAPPS) and the matching output PPS registers (RC3PPS/RC4PPS) so SCL and SDA are bidirectional on the same pins. [1]
- Optional (but recommended if you want the “I²C pad” electrical behavior): configure RC3I2C and RC4I2C (I²C-specific slew, pull-up strength selection, SMBus/I²C thresholds). [3], [4], [5]
Notes:
- RC3/RC4 are explicitly listed as I²C pads for this device family. [3]
- You still typically need external pull-up resistors on SCL and SDA (standard I²C practice).
🏁 Script executed:
#!/bin/bash
# Check if there's any comment or documentation about I2C pin mode in the code
sed -n '65,125p' pic18f26k83/i2c.c | grep -i "open.*drain\|odcon\|push.*pull\|i2c.*pin"Repository: waterloo-rocketry/rocketlib
Length of output: 118
🏁 Script executed:
#!/bin/bash
# Look at the entire w_status_t i2c_init function for any missed configuration
sed -n '125,155p' pic18f26k83/i2c.cRepository: waterloo-rocketry/rocketlib
Length of output: 839
Configure I2C pins for open-drain operation during bus recovery.
The PIC18F26K83 explicitly requires ODCON bits to enable open-drain behavior on I2C pins. Current bus recovery code at lines 79–87 and 101–104 actively drives SCL high (LATCbits.LATC3 = 1) instead of releasing it, which violates the I2C specification and risks bus contention if a slave is holding the line low.
During bus recovery, use TRIS register manipulation to implement open-drain: release the line high by setting TRIS=1, and pull low by setting TRIS=0 and LAT=0. Additionally, ensure ODCONC3 and ODCONC4 are set in the I2C initialization to enable open-drain on both SCL and SDA pins.
Open-drain bus recovery pattern
- TRISCbits.TRISC3 = 0; // SCL as output
+ TRISCbits.TRISC3 = 1; // Release SCL high by default
TRISCbits.TRISC4 = 0; // SDA as output
- LATCbits.LATC3 = 0; // SCL low
+ LATCbits.LATC3 = 0;
+ TRISCbits.TRISC3 = 0; // Drive SCL low
__delay_us(5);
- LATCbits.LATC3 = 1; // SCL high
+ TRISCbits.TRISC3 = 1; // Release SCL high
__delay_us(5);
- LATCbits.LATC3 = 0; // Hold SCL low
+ LATCbits.LATC3 = 0;
+ TRISCbits.TRISC3 = 0; // Hold SCL low
__delay_ms(35);
- LATCbits.LATC3 = 1; // Release SCL
+ TRISCbits.TRISC3 = 1; // Release SCL
__delay_us(10);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pic18f26k83/i2c.c` around lines 79 - 87, The bus-recovery code currently
drives SCL/SDA via LATC (LATCbits.LATC3/LATCbits.LATC4) which violates I2C
open-drain semantics; change recovery to use TRIS lines so "release high" = set
TRISCbits.TRISCx = 1 and "pull low" = set TRISCbits.TRISCx = 0 with
LATCbits.LATCx = 0 preloaded for low, and update the I2C initialization to
enable open-drain on the pins by setting ODCONC3 and ODCONC4; apply this pattern
in the recovery loop (the for-loop that pulses SCL) and where SDA/SCL are
toggled later so lines are released rather than actively driven high.
| w_status_t i2c_init(uint8_t clkdiv) { | ||
| // Disable module for configuration | ||
| I2C1CON0bits.EN = 0; | ||
|
|
||
| // Setup and verify clock configuration | ||
| CLKRCON = 0b10010000 | (clkdiv & 0x07); | ||
| CLKRCLK = 0b00000011; // MFINTOSC source |
There was a problem hiding this comment.
Validate clkdiv range explicitly before programming CLKRCON.
Line 126 masks out-of-range values (& 0x07), which can hide caller bugs and silently select unintended timing.
🛠️ Proposed fix
w_status_t i2c_init(uint8_t clkdiv) {
+ if (clkdiv > 7U) {
+ return W_IO_ERROR;
+ }
// Disable module for configuration
I2C1CON0bits.EN = 0;
@@
- CLKRCON = 0b10010000 | (clkdiv & 0x07);
+ CLKRCON = 0b10010000 | clkdiv;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| w_status_t i2c_init(uint8_t clkdiv) { | |
| // Disable module for configuration | |
| I2C1CON0bits.EN = 0; | |
| // Setup and verify clock configuration | |
| CLKRCON = 0b10010000 | (clkdiv & 0x07); | |
| CLKRCLK = 0b00000011; // MFINTOSC source | |
| w_status_t i2c_init(uint8_t clkdiv) { | |
| if (clkdiv > 7U) { | |
| return W_IO_ERROR; | |
| } | |
| // Disable module for configuration | |
| I2C1CON0bits.EN = 0; | |
| // Setup and verify clock configuration | |
| CLKRCON = 0b10010000 | clkdiv; | |
| CLKRCLK = 0b00000011; // MFINTOSC source |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pic18f26k83/i2c.c` around lines 121 - 127, The clkdiv argument passed to
i2c_init must be explicitly validated (allowed range 0–7) before programming
CLKRCON to avoid masking caller bugs; update the i2c_init function to check if
clkdiv > 7 and return an appropriate error w_status_t (e.g.,
failure/invalid-parameter) prior to writing CLKRCON/CLKRCLK and disabling EN, so
only validated values are ever used when setting CLKRCON.
| static w_status_t i2c_write(uint8_t address, const uint8_t *data, uint8_t len) { | ||
| // Verify bus state and prepare for transfer | ||
| w_status_t status = check_i2c_state(); | ||
| if (status != W_SUCCESS) { | ||
| return status; | ||
| } |
There was a problem hiding this comment.
Guard pointer parameters before dereference.
The API can dereference null pointers (data, value) when callers pass invalid inputs.
🛡️ Proposed fix
static w_status_t i2c_write(uint8_t address, const uint8_t *data, uint8_t len) {
+ if ((data == NULL) && (len > 0U)) {
+ return W_IO_ERROR;
+ }
// Verify bus state and prepare for transfer
w_status_t status = check_i2c_state(); static w_status_t i2c_read(uint8_t address, uint8_t *data, uint8_t len) {
+ if ((data == NULL) && (len > 0U)) {
+ return W_IO_ERROR;
+ }
// Verify bus state and prepare for transfer
w_status_t status = check_i2c_state(); w_status_t i2c_read_reg8(uint8_t address, uint8_t reg, uint8_t *value) {
+ if (value == NULL) {
+ return W_IO_ERROR;
+ }
// Verify bus state and prepare for transfer w_status_t i2c_read_reg16(uint8_t address, uint8_t reg, uint16_t *value) {
+ if (value == NULL) {
+ return W_IO_ERROR;
+ }
// Verify bus state and prepare for transferAlso applies to: 205-210, 288-293, 363-368
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pic18f26k83/i2c.c` around lines 154 - 159, Guard pointer parameters before
any dereference: in i2c_write check that 'data' is not NULL before using it and
return an appropriate w_status_t (e.g., W_ERR_INVALID_PARAM) after
check_i2c_state; apply the same pattern to the other I2C helpers referenced
(e.g., i2c_read — validate 'data' buffer, i2c_write_byte and i2c_read_byte —
validate any 'value' pointer/argument if it is dereferenced) so callers passing
NULL don't cause crashes. Locate these checks in the functions named i2c_write,
i2c_read, i2c_write_byte, i2c_read_byte and add early returns on invalid pointer
inputs with a clear error status.
| if (I2C1CON0bits.CSTR) { | ||
| __delay_us(I2C_STRETCH_DELAY); | ||
| timeout = 0; | ||
| } else if (timeout++ >= I2C_POLL_TIMEOUT) { | ||
| return W_IO_TIMEOUT; |
There was a problem hiding this comment.
Clock-stretch handling can bypass timeout and hang forever.
timeout is reset while CSTR is set, so a permanently stretched clock can cause an infinite loop.
🛠️ Proposed fix
- if (I2C1CON0bits.CSTR) {
- __delay_us(I2C_STRETCH_DELAY);
- timeout = 0;
- } else if (timeout++ >= I2C_POLL_TIMEOUT) {
+ if (I2C1CON0bits.CSTR) {
+ __delay_us(I2C_STRETCH_DELAY);
+ }
+ if (timeout++ >= I2C_POLL_TIMEOUT) {
return W_IO_TIMEOUT;
}- if (I2C1CON0bits.CSTR) {
- __delay_us(I2C_STRETCH_DELAY);
- timeout = 0;
- } else if (timeout++ >= I2C_POLL_TIMEOUT) {
+ if (I2C1CON0bits.CSTR) {
+ __delay_us(I2C_STRETCH_DELAY);
+ }
+ if (timeout++ >= I2C_POLL_TIMEOUT) {
return W_IO_TIMEOUT;
}Also applies to: 240-244
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pic18f26k83/i2c.c` around lines 183 - 187, The loop resets timeout whenever
I2C1CON0bits.CSTR is set, which allows a permanently stretched clock to prevent
reaching I2C_POLL_TIMEOUT and hang; change the logic in the polling loops (the
block checking I2C1CON0bits.CSTR, the timeout variable, I2C_STRETCH_DELAY and
the I2C_POLL_TIMEOUT comparison that returns W_IO_TIMEOUT) to stop resetting
timeout while CSTR is asserted — instead delay when CSTR is set but continue to
let timeout advance (or increment a dedicated elapsed counter while delaying) so
the timeout check can eventually return W_IO_TIMEOUT; apply the same fix to the
repeated block around the other occurrence (the one referenced similarly at
lines ~240-244).
| // Configure acknowledgment behavior | ||
| I2C1CON1bits.ACKDT = 0; // ACK bytes | ||
| if (len == 1) { | ||
| I2C1CON1bits.ACKCNT = 1; // NACK last byte | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
PIC18F26K83 I2C ACKCNT register behavior datasheet documentation
💡 Result:
On the PIC18F26K83 I2C peripheral (as documented in the PIC18(L)F25/26K83 datasheet DS40001943C), ACKCNT is not a counter—it is the Acknowledge bit value that hardware will put on SDA when I2CxCNT has reached 0 (i.e., at the “last byte” boundary of a counted receive). (pdf.elecfans.com)
What selects ACKDT vs ACKCNT
When the module is in a receiving mode, the ACK bit driven on the 9th clock depends on I2CxCNT:
- If
I2CxCNT != 0: hardware uses ACKDT, and then decrementsI2CxCNT. (pdf.elecfans.com) - If
I2CxCNT == 0: hardware uses ACKCNT instead (and setsCNTIF). (pdf.elecfans.com)
This same “if CNT==0 use ACKCNT else use ACKDT and decrement CNT” behavior is described in the step-by-step bus sequences in the datasheet. (pdf.elecfans.com)
Master receive: why ACKCNT usually must be 1 (NACK)
In master reception, the datasheet explicitly warns that if you don’t set ACKCNT = 1, the master may never send a NACK when I2CxCNT becomes zero, and because no NACK appears on the bus, the hardware will also not assert a Stop condition (bus can stall). (pdf.elecfans.com)
So the intended/common usage is:
ACKDT = 0(ACK) for intermediate bytesACKCNT = 1(NACK) for the final byte whenI2CxCNTreaches 0 (pdf.elecfans.com)
Citations:
- 1: https://pdf.elecfans.com/ACTEL/PIC18F26K83-I--SS.html
- 2: https://pdf.elecfans.com/ACTEL/PIC18F26K83-I--SS.html
- 3: https://pdf.elecfans.com/ACTEL/PIC18F26K83-I--SS.html
- 4: https://pdf.elecfans.com/ACTEL/PIC18F26K83-I--SS.html
- 5: https://pdf.elecfans.com/ACTEL/PIC18F26K83-I--SS.html
- 6: https://pdf.elecfans.com/ACTEL/PIC18F26K83-I--SS.html
🏁 Script executed:
# First, find and examine the i2c.c file
find . -name "i2c.c" -type f | head -5Repository: waterloo-rocketry/rocketlib
Length of output: 90
🏁 Script executed:
# Look for the context around the flagged lines
fd "i2c.c" -type f --exec head -250 {} \; 2>/dev/null | tail -100Repository: waterloo-rocketry/rocketlib
Length of output: 53
🏁 Script executed:
cat -n ./pic18f26k83/i2c.c | head -260 | tail -80Repository: waterloo-rocketry/rocketlib
Length of output: 2210
🏁 Script executed:
# Get the full context around lines 200-240 to understand the I2C read flow
cat -n ./pic18f26k83/i2c.c | sed -n '200,240p'Repository: waterloo-rocketry/rocketlib
Length of output: 1223
🏁 Script executed:
# Search for I2CxCNT usage and initialization
rg "I2C1CON|I2CxCNT|ACKCNT|ACKDT" ./pic18f26k83/i2c.c -nRepository: waterloo-rocketry/rocketlib
Length of output: 894
🏁 Script executed:
# Search for I2C protocol documentation or comments in the file
rg "I2C|master|receive|NACK" ./pic18f26k83/i2c.c -i -B2 -A2 | head -80Repository: waterloo-rocketry/rocketlib
Length of output: 2161
🏁 Script executed:
# Check if there are any comments or documentation explaining ACKCNT behavior
grep -n "ACKCNT\|ACKDT" ./pic18f26k83/i2c.c -B1 -A1Repository: waterloo-rocketry/rocketlib
Length of output: 555
🏁 Script executed:
# Look at the entire I2C initialization and understand the register defaults
cat -n ./pic18f26k83/i2c.c | sed -n '135,155p'Repository: waterloo-rocketry/rocketlib
Length of output: 715
Initialize ACKCNT explicitly for all read transactions.
The original concern is valid. For multi-byte reads, when I2CxCNT reaches 0, the hardware uses ACKCNT to determine the ACK bit for the final byte. According to the PIC18F26K83 datasheet, ACKCNT must be set to 1 (NACK) for proper I2C master receive termination; failure to set it can cause bus stall.
The proposed fix, however, is incorrect. Setting ACKCNT = (len == 1U) ? 1U : 0U would clear ACKCNT to 0 for multi-byte reads, causing the final byte to be ACK'd instead of NACK'd, which violates the I2C protocol. ACKCNT should be set to 1 for all read operations.
🛠️ Corrected fix
// Configure acknowledgment behavior
I2C1CON1bits.ACKDT = 0; // ACK bytes
- if (len == 1) {
- I2C1CON1bits.ACKCNT = 1; // NACK last byte
- }
+ I2C1CON1bits.ACKCNT = 1; // Always NACK last byte🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pic18f26k83/i2c.c` around lines 220 - 224, The ACKCNT field must be
explicitly set to 1 for every master read to ensure the final byte is NACKed;
change the read transaction code that currently conditionally sets
I2C1CON1bits.ACKCNT based on len (the block around I2C1CON1bits.ACKDT and the if
(len == 1) { I2C1CON1bits.ACKCNT = 1; } ) so that I2C1CON1bits.ACKCNT = 1 is
assigned unconditionally for all receive operations (use the same location where
ACKDT is configured).
5 participants
Summary by CodeRabbit
Refactor
Documentation