boards: slstk3401a: add support (v2) by basilfx · Pull Request #8630 · RIOT-OS/RIOT

basilfx · 2018-02-23T16:31:11Z

Contribution description

This PR add supports for the Silicon Labs SLSTK3401a. The files are similar to the SLTB001a and STK3600 and STK3700 already supported. The main difference with the STK3x00 is that is has a second series MCU and it has a memory LCD (supported by U8g2).

For convenience, this is the diff between STK3600 and SLSTK3401a board files.

Issues/PRs references

#8585
#8520

aabadie

Made a pass of review and found minor things.

aabadie · 2018-02-27T14:21:51Z

boards/slstk3401a/Makefile.dep

@@ -0,0 +1,9 @@
+ifneq (,$(filter saul_default,$(USEMODULE)))
+    USEMODULE += saul_gpio


indentation should be 2 spaces here

aabadie · 2018-02-27T14:23:00Z

boards/slstk3401a/Makefile.features

+# The board MPU family (used for grouping by the CI system)
+FEATURES_MCU_GROUP = cortex_m4_2
+
+include $(RIOTCPU)/efm32/Makefile.features


just comparing to nucleos and I think if a '-' at the beginning of the line is required

According to this link, it should be prefixed if you don't care if the file exists. But I do :-)

I see that others have it, but I wonder why the cpu include is somewhat optional...

Fixed it though.

I do think the optional include in the other boards is a bug though.

aabadie · 2018-02-27T14:25:08Z

boards/slstk3401a/include/board.h

+ * Connection to the on-board temperature/humidity sensor (Si7021).
+ * @{
+ */
+#ifndef SI7021_ENABLED


I don't think this is the way to go. Just bind this to the fact that the module is loaded or not, e.g. use MODULE_SI7021, or even MODULE_SI70XX

So in fact this part is not needed.

Removed this part.

aabadie · 2018-02-27T14:26:30Z

boards/slstk3401a/board.c

+    board_common_init();
+
+    /* initialize the Si7021 sensor */
+#if SI7021_ENABLED


use:

#ifdef MODULE_SI70XX

instead. This flag is set automatically when one adds USEMODULE += si7021 in the application makefile.

I didn't see this comment. IMHO this is not needed at all, since the inclusion of the driver is not really optional, the device is physically attached to the board and cannot be detached or deactivated, thus it belongs to the board and should be initialised whenever it's used.

The iotlab-m3 board has also some sensors attached, maybe you can take a look there to see how it's implemented, but as far as I can tell, they are only initialised when they're used through auto_init.

Another option would be to add a params file so you can access the sensor through SAUL using the default example.

since the inclusion of the driver is not really optional, the device is physically attached to the board

yes it is, if the application doesn't load the driver module, there's no need for initializing and setting the pin that control it on the board.

I just got the purpose of this, made my comment in my review.

basilfx · 2018-03-06T23:09:16Z

Fixed comments and rebased PR.

jia200x · 2018-03-07T08:46:10Z

Don't have this board to test, but code-wise looks fine to me

basilfx · 2018-03-07T09:12:04Z

@haukepetersen Do you have access to this board, and willing to test this one?

basilfx · 2018-03-08T08:58:44Z

Don't have this board to test, but code-wise looks fine to me

@jia200x I can also provide you with remote access (via SSH), if you want to try?

jia200x · 2018-03-08T23:03:55Z

@jia200x I can also provide you with remote access (via SSH), if you want to try?

Thanks for the offer. Unfortunately I won't be in the office until next monday, so I think would be nice if someone else can test it before :)

basilfx · 2018-03-13T14:59:03Z

@jia200x Are you still up for testing this one? So far nobody had the time :-)

jia200x · 2018-03-13T15:27:56Z

Tested, works as expected.

jia200x

ACK

jia200x · 2018-03-13T15:29:44Z

let's wait for @aabadie and @kYc0o

kYc0o

I found some small style issues but besides it's OK.

kYc0o · 2018-03-16T12:51:17Z

boards/slstk3401a/board.c

+    /* perform common board initialization */
+    board_common_init();
+
+#ifdef MODULE_SI70XX


is it possible to not use this module physically on the board? I mean, that you can disable it somehow so you need this guard?

Well, removing this condition will always enable the sensor, even if you don't use it. I would therefore not want to remove it.

So what you mean is that this pin can turn on/off the sensor? If that's the case, then this is a kind of "software detachment".

IMHO it looks a bit strange to have it here, but I don't have in mind right now a better way to integrate this functionality.

a better way to integrate this functionality.

It could be done using the driver params, as you suggested before. But this is something that is not described in the sensor datasheet, a kind of external switch, only available on that board. So the actual solution is the best one I think.

Just to clarify: this pin is not directly connected to the enable line of the Si7021: it's connected to a board-specific IC that will toggle VDD for the Si7021. The Si7021 does not have one.

So to use this Si7021, you have to enable this pin to supply it VDD. The reason to disable it, is to not take into account its consumption when profiling power consumption. The user manual calls it the enable pin, but it could also have been called toggle-power pin :-)

@basilfx Yes is what I thought, a real physical software controlled switch. That's why I didn't have an idea on how to integrate it, but I guess we should leave it as it is here and if the case comes across again we find a better integration.

kYc0o · 2018-03-16T12:52:43Z

boards/slstk3401a/include/periph_conf.h

+
+#include "periph_cpu.h"
+
+#include "em_cmu.h"


Can you put all the includes together?

kYc0o · 2018-03-16T12:53:20Z

boards/slstk3401a/board.c

+#include "board.h"
+#include "board_common.h"
+
+#include "periph/gpio.h"


I think the space in between is not necessary.

kYc0o · 2018-03-16T12:53:50Z

boards/slstk3401a/include/board.h

+
+#include "cpu.h"
+
+#include "periph_conf.h"


kYc0o · 2018-03-16T12:57:16Z

You can squash the changes so the PR is ready.

aabadie

My comments have been addressed, ACK

basilfx · 2018-03-16T15:31:21Z

Squashed and rebased.

kYc0o · 2018-03-16T15:34:20Z

ACK. Merge when Murdock agrees.

basilfx · 2018-03-16T15:37:47Z

Thank you all!

basilfx requested review from jia200x and kYc0o February 23, 2018 16:31

basilfx added Platform: ARM Platform: This PR/issue effects ARM-based platforms CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: boards Area: Board ports labels Feb 23, 2018

aabadie requested changes Feb 27, 2018

View reviewed changes

basilfx force-pushed the feature/efm32_slstk3401a branch from d05b362 to 42ae8c2 Compare March 6, 2018 23:08

jia200x approved these changes Mar 13, 2018

View reviewed changes

kYc0o requested changes Mar 16, 2018

View reviewed changes

aabadie approved these changes Mar 16, 2018

View reviewed changes

basilfx added 3 commits March 16, 2018 16:26

cpu/efm32: efm32pg1b: add vendor headers

cbca0cc

cpu/efm32: efm32pg1b: add support

9117fe5

boards: slstk3401a: add support

995973f

basilfx force-pushed the feature/efm32_slstk3401a branch from 42ae8c2 to 995973f Compare March 16, 2018 15:30

kYc0o approved these changes Mar 16, 2018

View reviewed changes

basilfx merged commit 4e84530 into RIOT-OS:master Mar 16, 2018

basilfx deleted the feature/efm32_slstk3401a branch March 16, 2018 15:38

basilfx mentioned this pull request Mar 16, 2018

boards: efm32: adapt pr feedback for other boards #8794

Merged

This was referenced Mar 20, 2018

boards: slstk3402a: add support #8802

Merged

boards: slwstk6000b: add support #8824

Merged

boards: stk3200: add support #8832

Closed

basilfx mentioned this pull request Oct 1, 2019

boards/stk3200: add support #12353

Merged

		@@ -0,0 +1,9 @@
		ifneq (,$(filter saul_default,$(USEMODULE)))
		USEMODULE += saul_gpio


		#include "periph_cpu.h"

		#include "em_cmu.h"


		#include "cpu.h"

		#include "periph_conf.h"

Conversation

basilfx commented Feb 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Issues/PRs references

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basilfx commented Mar 6, 2018

Uh oh!

jia200x commented Mar 7, 2018

Uh oh!

basilfx commented Mar 7, 2018

Uh oh!

basilfx commented Mar 8, 2018

Uh oh!

jia200x commented Mar 8, 2018

Uh oh!

basilfx commented Mar 13, 2018

Uh oh!

jia200x commented Mar 13, 2018

Uh oh!

jia200x left a comment

Choose a reason for hiding this comment

Uh oh!

jia200x commented Mar 13, 2018

Uh oh!

kYc0o left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

basilfx Mar 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kYc0o commented Mar 16, 2018

basilfx commented Feb 23, 2018 •

edited

Loading

basilfx Mar 16, 2018 •

edited

Loading