cpu/stm32: fix periph_i2c for F1, F2, L1 and F4 families#20100
cpu/stm32: fix periph_i2c for F1, F2, L1 and F4 families#20100maribu merged 2 commits intoRIOT-OS:masterfrom
Conversation
maribu
added
Type: bug
maribu
requested review from
DipSwitch,
aabadie,
fjmolinas and
vincent-d
as code owners
github-actions
bot
added
the
Platform: ARM
maribu
force-pushed
the
cpu/stm32/periph/i2c_2.c
branch
2 times, most recently
from
34adcdc to
998a7eb
Compare
benpicco
left a comment
benpicco
left a comment
There was a problem hiding this comment.
Looks good, the DEBUG -> DEBUG_PUTS change obscures the diff a bit, would have been better as a separate commit (but you don't have delaminate that 😉)
Do you know why the open-drain configuration was chosen?
cpu/stm32/periph/i2c_2.c
Outdated
| break; | ||
| case I2C_SPEED_LOW: | ||
| /* 10Kbit/s */ | ||
| ccr = i2c_config[dev].clk / 20000; |
There was a problem hiding this comment.
If i2c_config[dev].speed would just be the speed in Hz you could do
| ccr = i2c_config[dev].clk / 20000; | |
| ccr = i2c_config[dev].clk / (2 * i2c_config[dev].speed); |
for arbitrary I2C frequencies. (but this is unrelated to this PR)
There was a problem hiding this comment.
I added this as a separate PR. The change is IMO trivial enough to not require a separate review.
maribu
force-pushed
the
cpu/stm32/periph/i2c_2.c
branch
from
998a7eb to
ecf2f2b
Compare
I split the changes on the DEBUG output out as a separate commit |
maribu
force-pushed
the
cpu/stm32/periph/i2c_2.c
branch
from
ecf2f2b to
10641ae
Compare
| #include "irq.h" | ||
| #include "mutex.h" | ||
| #include "pm_layered.h" | ||
| #include "panic.h" |
There was a problem hiding this comment.
panic.h seems to be used eg.: l.:516 core_panic(PANIC_GENERAL_ERROR, "I2C FAULT");
| else { | ||
| /* End transmission */ | ||
| DEBUG("[i2c] write_bytes: Ending transmission\n"); | ||
| DEBUG_PUTS("[i2c] i2c_write_bytes(): Ending transmission"); |
There was a problem hiding this comment.
why not stay with DEBUG
afaik you optimized the behavior of printf -> DEBUG on AVR (I know this isn't AVR but using the same style even on definitely not AVR leads to consistency).
a short visit to godbolt revealed printf is optimized to puts anyway ( even with gcc -O0) (probably not for AVR with your flashprint optimization )
There was a problem hiding this comment.
DEBUG() most of the time prints (something along the line of) stacksize too small for debugging using puts(). But since puts() apparently did work, it can be used without the stack test.
There was a problem hiding this comment.
#ifdef DEVELHELP
#include "cpu_conf.h"
#define DEBUG_PRINT(...) \
do { \
if ((thread_get_active() == NULL) || \
(thread_get_active()->stack_size >= \
THREAD_EXTRA_STACKSIZE_PRINTF)) { \
printf(__VA_ARGS__); \
} \
else { \
puts("Cannot debug, stack too small. Consider using DEBUG_PUTS()."); \
} \
} while (0)
#else
#define DEBUG_PRINT(...) printf(__VA_ARGS__)
#endif
wat
- boot the I2C after init in low power mode
- otherwise I2C will consume more power until the first time it is
used, which is surprising
- STM32 F1 only: reconfigure SCL and SDA as GPIOs while the I2C
peripheral is powered down
- When the I2C peripheral is not clocked, it drives SCL and SDA
down. This will dissipate power across the pull up resistor.
maribu
force-pushed
the
cpu/stm32/periph/i2c_2.c
branch
from
6d2782e to
3002f1e
Compare
|
🎉 thx :-) |
5 participants
Contribution description
Testing procedure
master, the MCU should use more power on F2, F4, L1 whenperiph_i2cis enabled but not used compared to this PR, as the peripheral is only put into low power mode after the first usemasterthe idle level of SCL and SDA on F1 is low after the first use of the I2C peripheral, with this PR it is highmaster, thetests/periph/selftest_shieldwill fail for thenucleo-f103rbdue to I2C communication errors after the first timei2c_release()is called. With this PR, the I2C communication should succeed consistentlyI tested this locally in a branch that also contains the commits of this PR and of #20084:
~/Repos/software/RIOT/stm32_spi/tests/periph/selftest_shield % make BOARD=nucleo-f103rb flash test-with-config -j~/Repos/software/RIOT/stm32_spi/tests/periph/selftest_shield % make BOARD=nucleo-l152re flash test-with-config -j~/Repos/software/RIOT/stm32_spi/tests/periph/selftest_shield % make BOARD=nucleo-f446re flash test-with-config -jFor the
nucleo-f103rbI also use a logic analyzer. Here the output when runningmaster:(Note that the idle level goes to low after the first transaction during
pcf857x_init()that consistets of one write and one read.) Subsequently, the I2C fails to send the start condition. This inmasterhowever resulted in the peripheral to be re-initialized, after which the I2C will work until the nexti2c_release(). So users may have worked around this bug by just retrying the I2C transfer.Here the output when running this PR:
Note that the idle level now is high.
Issues/PRs references
#20071