Add support for the Sillicon Labs Thunderboard Sense (updated) #7929

basilfx · 2017-11-01T23:01:31Z

So here is my fourth attempt of adding EFM32 to RIOT-OS. I choose to open a new PR, since a lot has changed since my last attempt (#5652). It is a similar PR to #5652 and #4824 and the comments of PR #4722 still apply.

This PR adds support for the Thunderboard Sense (SLTB001A) by Silicon Labs. It's still a vendor-library supported version, but this time this vendor library is added as an external package. It's split up in four commits, and this is as small as I can get it.

Most drivers have been implemented. The best example to check out is one with SAUL enabled, since drivers for the Si7021 and BMP280 are available (it does depend on #7921 for the time being).

Board description: here
Benchmarks: here

basilfx · 2017-11-01T23:07:18Z

To address some of the previous comments:

the emlib files need to be excluded from doxygen

The emlib files are now a package, the other vendor files are in the vendor/ folder now. Usage to emlib is limited to peripheral drivers and board files only.

the license header of the emlib files needs to be included into the license patterns

The emlib files are now a package.

for the peripheral drivers (except SPI and I2C for now): remove the #ifdef xx_NUMOF guards around the file, since the peripheral remodeling these are not longer needed

Turns out that this is still needed until selective compilation of peripherals is included (cannot find PR number this quick).

all vendor supplied headers should be moved to a vendor include (sub)dir as we did for all other CPUs. This helps to separate things and also tells doxygen not to look at them

Done.

I think it would be much better to provide the emlib as a pkg. As it seems, SiLabs provides it in their official github (https://github.com/SiliconLabs/Gecko_SDK/tree/master/platform/emlib/src), so this should be easy to do

Done, but using my own mirror, since Silicon Labs is not updating their repository anymore.

basilfx · 2017-11-01T23:12:27Z

So, this PR is the start of fixing #1123. If this PR gets merged, I will open additional PR's to add support for the following boards (I own most of them):

SLSTK3401A — EFM32 Pearl Gecko PG1
SLSTK3402A — EFM32 Pearl Gecko PG12
SLTB001A — EFR32 Mighty Gecko
STK3200 — EFM32 Zero Gecko
STK3600 — EFM32 Leopard Gecko
STK3700 — EFM32 Giant Gecko
STK3800 — EFM32 Wonder Gecko

Optionally (will replace existing one):

SLWSTK6220A — EZR32 Wonder Gecko

In total, I can add support for 715 additional MCUs (34 families). That is excluding devices with less than 4kb of flash and 8kb of memory.

kaspar030 · 2017-11-02T11:52:40Z

Would you mind rebaseing this to #7241? I hope to be able to merge it today...

basilfx · 2017-11-02T12:27:47Z

@kaspar030 Yeah sure, I'll adapt this PR after #7241 gets merged.

haukepetersen · 2017-11-03T09:39:34Z

Awesome, and thanks for sticking with this, despite the trouble I might have caused you :-)

Will review the PR now. Already tested it (with #7921 being merged) and it works like a charm...

haukepetersen

Looking good! Some remarks (of which some are probably subjective and debatable..).

haukepetersen · 2017-11-03T09:38:42Z

boards/sltb001a/include/gpio_params.h

+    {
+        .name = "Button 1",
+        .pin = PB0_PIN,
+        .mode = GPIO_IN_PU


you could add .flags = SAUL_GPIO_INVERTED here to reflect active low circuit of the buttons...

Done, thanks for de addition.

haukepetersen · 2017-11-03T09:45:35Z

boards/sltb001a/Makefile.features

@@ -0,0 +1,19 @@
+# Put defined MCU peripherals here (in alphabetical order)
+FEATURES_PROVIDED += periph_adc
+FEATURES_PROVIDED += periph_cpuid


could you adapt the list of features to the changes introduced in #7880?

So move all features that don't depend on any board specific configuration (e.g. cpuid) to the (possibly shared) CPU's makefile.features, which then needs to be included from this file.

I only moved periph_cpuid.

haukepetersen · 2017-11-03T09:48:11Z

boards/sltb001a/board.c

+#include "periph/gpio.h"
+#include "periph/i2c.h"
+
+#if BMP280_ENABLED || CCS811_ENABLED || ICM_20648_ENABLED || SI1133_ENABLED || SI7021_ENABLED || SI7210A_ENABLED || RGB_LED1_ENABLED || RGB_LED2_ENABLED || RGB_LED3_ENABLED || RGB_LED4_ENABLED


this line seems to slightly exceed the 80 char line length... :-)

haukepetersen · 2017-11-03T09:51:58Z

boards/sltb001a/board.c

+#if BMP280_ENABLED || CCS811_ENABLED || ICM_20648_ENABLED || SI1133_ENABLED || SI7021_ENABLED || SI7210A_ENABLED || RGB_LED1_ENABLED || RGB_LED2_ENABLED || RGB_LED3_ENABLED || RGB_LED4_ENABLED
+static inline void board_usleep(uint32_t delay)
+{
+    delay = (delay * 1000) / (1000 / (SystemCoreClock / 1000 / 1000));


delay = (delay * (SystemCoreClock / 1000000))?

Also, divide this value by 2, as the loop takes two instructions (nop and inc and compare)?

I think I have it now:

MHz / 1000 / 1000 -> instructions per usec -> multiply by delay to get number of instructions to skip, divided by two (compare and inc).

haukepetersen · 2017-11-03T09:56:25Z

boards/sltb001a/include/board.h

+#ifndef BMP280_ENABLED
+#define BMP280_ENABLED          (1)
+#endif
+#define BMP280_I2C              (0)


here and below: this should better be I2C_DEV(0)

haukepetersen · 2017-11-03T10:37:12Z

cpu/efr32mg1p/Makefile.include

+
+# enable memory protection unit
+USEMODULE += mpu_stack_guard
+USEMODULE += pm_layered


shouldn't pm_layered be included from efm32_common?

For the STM32F1/F2/F4 it is also on per-CPU base. Any preference?

The STMs do this on a per-CPU base, as we simply didn't touch all of the CPUs, yet. In the end this will be done in stm32_common. So as I also see all EFM32 CPUs use that module, I would put the usemodule define there.

haukepetersen · 2017-11-03T10:38:11Z

cpu/efr32mg1p/Makefile.include

+export CPU_ARCH = cortex-m4
+export CPU_FAM = efr32mg1p
+
+# enable memory protection unit


any reason for enabling memory protection as default for this CPU? Shouldn't this be something that is enabled by the user?

I'll remove it from here (I think I found it a good idea to enable it by default back then).

I think it makes sense to remove it. This again a matter of expectation: CPUs should only include what their implementation really needs. Everything else should be up to external modules/applications to define.

haukepetersen · 2017-11-03T10:40:46Z

cpu/efr32mg1p/periph/Makefile

@@ -0,0 +1,3 @@
+MODULE = periph
+
+include $(RIOTBASE)/Makefile.base


probably best to wait for #7241 and adapt before merging.

Not yet included, but will do once merged.

haukepetersen · 2017-11-03T10:42:56Z

cpu/efr32mg1p/vectors.c

+WEAK_DEFAULT void isr_fpueh(void);
+
+/* interrupt vector table */
+ISR_VECTOR(1) const isr_t vector_cpu[CPU_IRQ_NUMOF] = {


I would prefer to use the indexed notation here, e.g.

[ 0] = isr_em, ... [ 2] = isr_wdog0, ...

haukepetersen · 2017-11-03T10:44:10Z

pkg/emlib/Makefile

@@ -0,0 +1,11 @@
+PKG_NAME=emlib
+PKG_URL=https://github.com/basilfx/emlib


just wondering: as we use your fork of the emlib, couldn't we simply apply the patches directly to your fork and skip them in RIOT?

That's fine. I'll rename it, remove the patches and fix the URL.

basilfx · 2017-11-05T16:59:59Z

Thanks @haukepetersen for the review. I have fixed almost all feedback, and improved a few other things:

Added hwrng (not supported by sltb001a, but by newer boards such as the slstk3402a).
Documentation fixes.
Tooling fixes.

I think #7241 is the only thing remaining, if you agree :-)

aabadie · 2017-11-05T17:17:33Z

boards/sltb001a/Makefile.dep

+ifneq (,$(filter saul_default,$(USEMODULE)))
+    USEMODULE += saul_gpio
+    USEMODULE += bmp280
+    USEMODULE += si70xx


once #7296 is merged, here you can use the exact reference of the sensor (si7021 I think)

And included :-)

haukepetersen · 2017-11-06T09:27:43Z

Nice. #7241 is on its way, I expect it to be merged today.

basilfx · 2017-11-06T21:41:46Z

Support for #7241 added and some other fixes have been pushed.

There is one problem remaining: due to #7241, rtc.c is compiled when needed, but for some generations, I have rtc_gemstone.c. Since the SLTB001A is a Gemstone CPU, RTC will not work anymore.

What is the best way to fix this? (cc @kaspar030)?

(I see a similar case for kinetis_common/periph/hwrng_rngb.c, but I do not understand how this exception is compiled into RIOT-OS.)

Other than that, it should be ready.

kaspar030 · 2017-11-07T10:05:07Z

What is the best way to fix this? (cc @kaspar030)?

One way would be to have a different file for each rtc implementation, and then make "periph_rtc" depend on the right one (e.g., "periph_rtc_gemstone").
A good place for that dependency would probably be in cpu/$(CPU)/Makefile.dep.

(I see a similar case for kinetis_common/periph/hwrng_rngb.c, but I do not understand how this exception is compiled into RIOT-OS.)

hwrng_rngb is currently unused...

kaspar030 · 2017-11-07T10:06:31Z

boards/sltb001a/Makefile.include

+# setup serial terminal
+include $(RIOTMAKE)/tools/serial.inc.mk
+
+# include board dependencies


remove this here, the file is included by the main Makefile.dep

kaspar030 · 2017-11-07T10:07:21Z

boards/sltb001a/Makefile.dep

+endif
+
+# i2c is required for environmental sensors
+USEMODULE += periph_i2c


The sensors should specify their dependencies themselves, in drivers/Makefile.dep.

The board itself needs i2c for enabling the sensors (there is an additional chip on that board that is used as a GPIO expander).

I'll change the comment to reflect that situation.

haukepetersen

just this one thing and @kaspar030's last comment, then I think we should be ready to go!

haukepetersen · 2017-11-09T10:28:56Z

cpu/efm32_common/periph/rtc.c

+#include "em_cmu.h"
+#include "em_rtc.h"
+
+#if defined(RTC_COUNT) && RTC_COUNT > 0


I think these guards should be dropped now, as the RTC is only build if the feature is provided by the board/cpu, which again should only be the case if a valud RTC configuration is present...

haukepetersen · 2017-11-09T10:29:43Z

cpu/efm32_common/periph/pwm.c

+#include "em_timer_utils.h"
+
+/* guard file in case no PWM device was specified */
+#ifdef PWM_NUMOF


no need anymore, as this file is only build if a pwm feature is defined.

--> please check also all other periph implementation files for this.

basilfx · 2017-11-12T12:30:16Z

I've updated + rebased this PR. All comments should be fixed.

I also implemented a flashpage driver. New benchmarks/compile results here.

basilfx · 2017-11-12T14:01:40Z

It's finally green 😎

Last few commits include whitespace removal and Doxygen fixes.

If permitted, I'll squash it down into the first four commits.

haukepetersen

Looks good now! -> ACK

please squash

basilfx · 2017-11-13T15:24:52Z

Squashed! And it's green!

I'll add the Wiki page after this is merged.

@haukepetersen

Changes have been incorporated and @haukepetersen already approved.

basilfx · 2017-11-13T19:28:36Z

Reworded 102d85b because it's the efr32mg1p instead of efm32mg1p. Very sloppy of me!

No other changes besides a rebase.

haukepetersen · 2017-11-14T12:04:48Z

Excellent! Well done and thanks for all the work you put in!

basilfx added Platform: ARM Platform: This PR/issue effects ARM-based platforms Type: new feature The issue requests / The PR implemements a new feature for RIOT labels Nov 1, 2017

basilfx assigned haukepetersen, emmanuelsearch and kYc0o Nov 1, 2017

basilfx mentioned this pull request Nov 1, 2017

Add support for the Sillicon Labs Thunderboard Sense #5652

Closed

haukepetersen requested changes Nov 3, 2017

View reviewed changes

aabadie reviewed Nov 5, 2017

View reviewed changes

basilfx force-pushed the feature/thunderboard_v2 branch from e2496c9 to e9f1b3e Compare November 5, 2017 17:52

basilfx force-pushed the feature/thunderboard_v2 branch from e9f1b3e to e837fbd Compare November 6, 2017 21:35

kaspar030 previously requested changes Nov 7, 2017

View reviewed changes

haukepetersen requested changes Nov 9, 2017

View reviewed changes

basilfx force-pushed the feature/thunderboard_v2 branch from e837fbd to 0e3bd43 Compare November 12, 2017 12:28

basilfx added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Nov 12, 2017

basilfx force-pushed the feature/thunderboard_v2 branch 2 times, most recently from a847d4b to d60204c Compare November 12, 2017 13:55

haukepetersen approved these changes Nov 13, 2017

View reviewed changes

basilfx force-pushed the feature/thunderboard_v2 branch from d60204c to da40837 Compare November 13, 2017 15:11

basilfx added 4 commits November 13, 2017 20:27

pkg: emlib: add emlib package

5eaf198

cpu: efm32_common: add peripheral drivers

0537e50

cpu: efr32mg1p: add support

102d85b

boards: sltb001a: add support for thunderboard sense

a45d626

basilfx force-pushed the feature/thunderboard_v2 branch from da40837 to a45d626 Compare November 13, 2017 19:27

basilfx merged commit df3389b into RIOT-OS:master Nov 14, 2017

basilfx mentioned this pull request Nov 15, 2017

boards: ikea-tradfri: add support #8047

Merged

PeterKietzmann mentioned this pull request Nov 27, 2017

Support for EFM32 #1123

Closed

aabadie added this to the Release 2018.01 milestone Jan 18, 2018

basilfx mentioned this pull request Mar 16, 2018

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

Merged

basilfx deleted the feature/thunderboard_v2 branch January 9, 2020 20:42

		@@ -0,0 +1,3 @@
		MODULE = periph

		include $(RIOTBASE)/Makefile.base

		@@ -0,0 +1,11 @@
		PKG_NAME=emlib
		PKG_URL=https://github.com/basilfx/emlib

Add support for the Sillicon Labs Thunderboard Sense (updated) #7929

Add support for the Sillicon Labs Thunderboard Sense (updated) #7929

Conversation

basilfx commented Nov 1, 2017 • edited Loading

basilfx commented Nov 1, 2017 • edited Loading

basilfx commented Nov 1, 2017 • edited Loading

kaspar030 commented Nov 2, 2017

basilfx commented Nov 2, 2017

haukepetersen commented Nov 3, 2017

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basilfx Nov 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basilfx commented Nov 5, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen commented Nov 6, 2017

basilfx commented Nov 6, 2017 • edited Loading

kaspar030 commented Nov 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

haukepetersen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

basilfx commented Nov 12, 2017

basilfx commented Nov 12, 2017 • edited Loading

haukepetersen left a comment

Choose a reason for hiding this comment

basilfx commented Nov 13, 2017 • edited Loading

basilfx commented Nov 13, 2017

haukepetersen commented Nov 14, 2017

basilfx commented Nov 1, 2017 •

edited

Loading

basilfx commented Nov 1, 2017 •

edited

Loading

basilfx commented Nov 1, 2017 •

edited

Loading

basilfx Nov 3, 2017 •

edited

Loading

basilfx commented Nov 5, 2017 •

edited

Loading

basilfx commented Nov 6, 2017 •

edited

Loading

basilfx commented Nov 12, 2017 •

edited

Loading

basilfx commented Nov 13, 2017 •

edited

Loading