-
Notifications
You must be signed in to change notification settings - Fork 2k
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.
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
cpu/stm32: fix periph_i2c for F1, F2, L1 and F4 families #20100
Conversation
34adcdc
to
998a7eb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added this as a separate PR. The change is IMO trivial enough to not require a separate review.
998a7eb
to
ecf2f2b
Compare
I split the changes on the DEBUG output out as a separate commit |
ecf2f2b
to
10641ae
Compare
#include "mutex.h" | ||
#include "pm_layered.h" | ||
#include "panic.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
panic.h
seems to be used eg.: l.:516 core_panic(PANIC_GENERAL_ERROR, "I2C FAULT");
return ret; | ||
} | ||
if (flags & I2C_NOSTOP) { | ||
return 0; | ||
} | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
#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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed that in #20166
- 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.
6d2782e
to
3002f1e
Compare
🎉 thx :-) |
Contribution description
Testing procedure
master
, the MCU should use more power on F2, F4, L1 whenperiph_i2c
is enabled but not used compared to this PR, as the peripheral is only put into low power mode after the first usemaster
the 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_shield
will fail for thenucleo-f103rb
due 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 -j
For the
nucleo-f103rb
I 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 inmaster
however 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