boards/arduino-nano-33-ble-sense: add support for arduino-nano-33-ble-sense#20668
boards/arduino-nano-33-ble-sense: add support for arduino-nano-33-ble-sense#20668benpicco merged 1 commit intoRIOT-OS:masterfrom
Conversation
bmewen
requested review from
MrKevinWeiss,
jia200x and
leandrolanzieri
as code owners
github-actions
bot
added
Area: doc
bmewen
force-pushed
the
master
branch
4 times, most recently
from
b3c675d to
39d88d0
Compare
|
Sorry for the mess, it seems that I struggled a bit when I squashed my commits. And there was still some warnings remaining on the whitespaces that should be fixed now. Everything should be fine now, tests are running successfully and everything should be in the same commit. |
mguetschow
added
the
CI: ready for build
mguetschow
left a comment
mguetschow
left a comment
There was a problem hiding this comment.
Thanks, looks mostly good to me. I don't have the hardware around, so I couldn't test. Also I'm not very experienced with board porting yet, so just a soft-ACK from my side.
|
|
||
| #include "board.h" | ||
|
|
||
| #if defined(APDS9960) || defined(HTS221) || defined(lps22hb) |
There was a problem hiding this comment.
| #if defined(APDS9960) || defined(HTS221) || defined(lps22hb) | |
| #if defined(MODULE_APDS9960) || defined(MODULE_HTS221) || defined(MODULE_LPS22HB) |
that's what I would expect, but not fully sure. Same below.
There was a problem hiding this comment.
That's right I don't know why I wrote this that way its absolutely not correct
There was a problem hiding this comment.
Which makes me wonder whether it is actually needed, since it should have evaluated to false as is.
There was a problem hiding this comment.
Can you confirm it is really needed, or did it work with the old ifdef too?
| /** | ||
| * VDD needs to be set in order to have the sensors powered | ||
| * It also seems that if the internal I2C pins are not set as input, | ||
| * all the sensors connected will not work | ||
| */ |
There was a problem hiding this comment.
| /** | |
| * VDD needs to be set in order to have the sensors powered | |
| * It also seems that if the internal I2C pins are not set as input, | |
| * all the sensors connected will not work | |
| */ | |
| /* | |
| * VDD needs to be set in order to have the sensors powered. | |
| * It also seems that if the internal I2C pins are not set as input, | |
| * all the sensors connected will not work. | |
| */ |
Also, I would have expected the i2c driver to do this already automatically, maybe someone more knowledgeable than me in board porting can comment?
There was a problem hiding this comment.
Indeed, this had me surprised too - the I2C driver will re-configure those pins anyway, so it's weird you are setting them to input here.
There was a problem hiding this comment.
Maybe some of the drivers that were tested don't call the i2c_init function and therefore the pins are not correctly set (because the init_pins function is not called)?
https://github.com/RIOT-OS/RIOT/blob/master/cpu/nrf5x_common/periph/i2c_nrf52_nrf9160.c#L187
For example the lsm6dsxx driver does not call i2c_init: https://github.com/RIOT-OS/RIOT/blob/master/drivers/lsm6dsxx/lsm6dsxx.c#L52-L109
On the other hand there are drivers that call i2c_init such as this one: https://github.com/RIOT-OS/RIOT/blob/master/drivers/pca9633/pca9633.c#L69
When I worked on the AT24Cxx driver, it worked with the nRF52840 on the nRF52840DK, but the driver does not call i2c_init either. Maybe the default configuration for pins in the nRF52840DK is Input and since the arduino-nano-33-ble-sense has a bootloader, they might be configured differently (not Input)?
This is just a guess though, I don't have an Arduino Nano 33 BLE Sense board to test the hypothesis.
There was a problem hiding this comment.
i2c_init() is only called once in periph_common/init.c.
Since it's a shared bus, there is no point in the drivers calling that function.
there are drivers that call i2c_init
good find, that's a bug.
There was a problem hiding this comment.
The more you know 🤣
But now I don't have any ideas why it would behave like that either and I don't have any of the sensors to crosscheck it with the nRF52840DK (which does not need to have the GPIOs set explicitly for I2C to work).
| .scl = GPIO_PIN(0, 15), | ||
| .sda = GPIO_PIN(0, 14), |
There was a problem hiding this comment.
| .scl = GPIO_PIN(0, 15), | |
| .sda = GPIO_PIN(0, 14), | |
| .scl = SCL1, | |
| .sda = SDA1, |
since you have them already defined
There was a problem hiding this comment.
That was exactly why I defined those pins but for some reason I forgot to translate them there (I'm also using them in the board.c that's why I defined them if you're wondering)
bmewen
force-pushed
the
master
branch
2 times, most recently
from
633c1f4 to
11d3c8d
Compare
| @@ -0,0 +1,16 @@ | |||
| # Copyright (c) 2020 HAW Hamburg | |||
There was a problem hiding this comment.
Would you mind updating the copyright?
5 participants
Contribution description
This PR adds support for arduino-nano-33-ble-sense. Please note that it provides all the configuration to use the internal sensors and actuators but not all of them since they are not currently supported by RIOT (or only the older versions are supported such as LSM6DSXX).
This code is based on the code of the arduino-nano-33-ble as they share the same base (the sense version mostly provides sensors and actuators in addition).
Testing procedure