Skip to content
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

boards/arduino-nano-33-ble-sense: add support for arduino-nano-33-ble-sense #20668

Merged
merged 1 commit into from
May 23, 2024

Conversation

bmewen
Copy link
Contributor

@bmewen bmewen commented May 14, 2024

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

  1. Check the generated doc
  2. Run a few test examples

@github-actions github-actions bot added Area: doc Area: Documentation Area: boards Area: Board ports Area: Kconfig Area: Kconfig integration labels May 14, 2024
@bmewen bmewen force-pushed the master branch 4 times, most recently from b3c675d to 39d88d0 Compare May 14, 2024 12:46
@bmewen
Copy link
Contributor Author

bmewen commented May 14, 2024

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.

@benpicco benpicco requested a review from mguetschow May 14, 2024 15:28
@mguetschow mguetschow added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label May 15, 2024
@riot-ci
Copy link

riot-ci commented May 15, 2024

Murdock results

✔️ PASSED

368543b boards/arduino-nano-33-ble-sense: add support for arduino-nano-33-ble-sense

Success Failures Total Runtime
568 0 568 04m:29s

Artifacts

Copy link
Contributor

@mguetschow mguetschow left a comment

Choose a reason for hiding this comment

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

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
#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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That's right I don't know why I wrote this that way its absolutely not correct

Copy link
Contributor

Choose a reason for hiding this comment

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

Which makes me wonder whether it is actually needed, since it should have evaluated to false as is.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you confirm it is really needed, or did it work with the old ifdef too?

Comment on lines 30 to 34
/**
* 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
*/
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/**
* 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
.scl = GPIO_PIN(0, 15),
.sda = GPIO_PIN(0, 14),
.scl = SCL1,
.sda = SDA1,

since you have them already defined

Copy link
Contributor Author

@bmewen bmewen May 15, 2024

Choose a reason for hiding this comment

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

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 bmewen force-pushed the master branch 2 times, most recently from 633c1f4 to 11d3c8d Compare May 15, 2024 13:17
@@ -0,0 +1,16 @@
# Copyright (c) 2020 HAW Hamburg
Copy link
Contributor

Choose a reason for hiding this comment

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

Would you mind updating the copyright?

@bmewen bmewen mentioned this pull request May 21, 2024
6 tasks
@benpicco benpicco added this pull request to the merge queue May 23, 2024
Merged via the queue into RIOT-OS:master with commit 3f29f88 May 23, 2024
25 checks passed
@mguetschow mguetschow added this to the Release 2024.07 milestone Jun 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: boards Area: Board ports Area: doc Area: Documentation Area: Kconfig Area: Kconfig integration CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants