board: add hamilton board support by Hyungsin · Pull Request #9013 · RIOT-OS/RIOT

Hyungsin · 2018-04-24T18:50:35Z

This PR provides an initial driver for the Hamilton board [1,2]. @immesys.

Hamilton has samr21e18a SoC (Cortex M0+ CPU and AT86RF233 radio), HDC1080 (temperature and humidity sensors), FXOS8700 (3-axis accelerator and magnetometer), TMP006 (object temperature), APDS9007 (light sensor), Button, LED, and EKMB (PIR motiron sensor). It provides JTAG as the programming/debugging interface. Its idle current is ~6 uA when a 32 kHZ RTC is running.

Some sensors have not been merged yet: APDS9007 (#8989), EKMB (#7823), FXOS8700 (#8978).
Low power clock configuration has not been merged yet: #7826.

Therefore, this PR, as an initial support, provides a test application which tests operation of HDC1080, TMP006, Button, and LED without low idle current.

References

[1] https://bets.cs.berkeley.edu/publications/2017buildsys_hamiltondemo.pdf
[2] https://bets.cs.berkeley.edu/publications/2018sensys_hamilton.pdf

Hyungsin · 2018-06-15T19:56:09Z

Any review here?

MichelRottleuthner · 2018-06-18T09:54:03Z

Where to get one of these boards, is it publicly available somewhere?

Hyungsin · 2018-06-18T22:20:15Z

@MichelRottleuthner, Yes. You can get one from https://hamiltoniot.com/. I sent 5 hamiltons to @emmanuelsearch too.

Hyungsin · 2018-06-18T23:38:23Z

Since #8978 was merged, now this PR includes FXOS8700.

MichelRottleuthner

Thank you for this PR! Looks quite clean to me. I only found some minor things, see below. One thing I'm not sure about is the board-specific test application. I think gnerally it's nice to have a test app that makes use of all the boards features but I'm not convinced it actually adds additional value here. Most (if not all) of the things that are done in tests/board_hamilton should already be covered by the saul test, while being easier and more generic (less code; no need for adaption when adding additional sensors).

MichelRottleuthner · 2018-06-19T07:35:00Z

boards/hamilton/Makefile.dep

@@ -0,0 +1,15 @@
+ifneq (,$(filter gnrc_netdev_default netdev_default,$(USEMODULE)))
+  USEMODULE += at86rf233
+  USEMODULE += auto_init_gnrc_netif


I think the board itself shouldn't depend on auto_init_gnrc_netif - what if some application wants to use the radio (gnrc_netdev_default) but without auto-init?

What is a counter example? As saul_default includes "auto_init_saul" for sensor initialization, I think it is very convenient to include "auto_init_gnrc_netif" for radio initialization.

Yes it is convenient, but it should be upon the application to decide whether to use auto_init or not. Counter example: run grep -r --include "Makefile*" "USEMODULE += auto_init_gnrc_netif" from RIOT root dir and you will see that hamilton is the only board that adds this dependency, all other occurrences are from tests or examples (i.e. applications).

I changed the code, for consistency. But it is still not clear why "saul_default" includes auto_init in "RIOT-OS/Makefile.dep" but "netdev_default" does not. What is the reason?

MichelRottleuthner · 2018-06-19T07:35:55Z

boards/hamilton/Makefile.features

+FEATURES_PROVIDED += periph_rtt
+FEATURES_PROVIDED += periph_spi
+FEATURES_PROVIDED += periph_timer
+FEATURES_PROVIDED += periph_pm


alphabetical order

MichelRottleuthner · 2018-06-19T07:41:11Z

boards/hamilton/dist/reset.sh

@@ -0,0 +1,18 @@
+#!/bin/sh
+
+# This script resets a CC2538SF53 target using JLink called


CC2538SF53?

MichelRottleuthner · 2018-06-19T07:45:02Z

boards/hamilton/include/board.h

@@ -0,0 +1,151 @@
+/*
+ * Copyright (C) 2018 UC Berkeley


For consistency please stick to same format here and above. Also the previous remark stated 2016, are both correct?

done 2016 is more correct

MichelRottleuthner · 2018-06-19T07:46:57Z

boards/hamilton/include/board.h

+ *
+ * @file
+ * @brief       Board specific definitions for the Hamilton
+ *              board


can go to the line above

MichelRottleuthner · 2018-06-19T08:19:32Z

boards/hamilton/include/periph_conf.h

+#define PWM_MAX_CHANNELS    2
+/* for compatibility with test application */
+#define PWM_0_CHANNELS      PWM_MAX_CHANNELS
+#define PWM_1_CHANNELS      PWM_MAX_CHANNELS


Is this really needed? looks like some old compatibility code to me(?)

It is true, but no harm to have this just in case, right?

no harm, but we shouldn't add unnecessary code ;) I think you are safe to remove it here. The use of this define was removed some time ago.

OK. I resolved the issue. But FYI, samr21-xpro driver also has this part.

MichelRottleuthner · 2018-06-19T08:57:19Z

cpu/sam0_common/periph/cpuid.c

 #include <string.h>

 #include "periph/cpuid.h"
+#include "board.h"


It is related because fb_eui64 is defined in board.h.

sorry, I actually meant the same line in tmp006.h

MichelRottleuthner · 2018-06-19T08:58:32Z

sys/luid/luid.c

+    uint8_t *out = (uint8_t *)buf;
+    memcpy(out, fb_eui64, 8);
+#else
 #if CPUID_LEN


use #else if here

MichelRottleuthner · 2018-06-19T08:59:37Z

sys/luid/luid.c

        out[i % len] ^= cid[i];
    }
 #endif
+#endif


when using #else if above, one of the #endif lines can be removed here

MichelRottleuthner · 2018-06-19T09:04:30Z

boards/hamilton/include/periph_conf.h

+ * CORECLOCK = ((PLL_MUL + 1) * 1MHz) / PLL_DIV
+ *
+ * NOTE: The PLL circuit does not run with less than 32MHz while the maximum PLL
+ *       frequency is 96MHz. So PLL_MULL must be between 31 and 95!


MichelRottleuthner · 2018-06-19T09:37:42Z

I don't have a board here to actually test. @emmanuelsearch can you or someone at FU test this on real hardware? Or can you spare one of the five boards you have for some time? (I assume @tcschmidt would be willing to ship it from FU to HAW and back)

Hyungsin · 2018-06-19T22:53:57Z

@MichelRottleuthner, I reflected most comments.

Regarding the test application, I do think that providing a complete test application is good for users to experience full features of the board quickly, without tedious trials and errors. This is to mitigate any misunderstanding and hasty frustration.

The hamilton is designed for low-power sensing and wireless reporting, which needs to be shown in this test application. I'm thinking working on this application further to show that hamilton periodically wakes up, senses all information, and broadcasts a packet including the information. The idle current should be ~6uA.

If redundancy is only thing that matters, please allow me to maintain this application :)

MichelRottleuthner · 2018-06-20T09:20:45Z

drivers/include/tmp006.h

 #include <stdint.h>
 #include <stdbool.h>
 #include "periph/i2c.h"
+#include "board.h"


This is used for TMP006_USE_LOW_POWER, right? It looks a bit misplaced to me, that the driver of a temperature sensor needs to know something about the board it is used on, except for it's parameters. Is it really needed to define this in board.h? I mean, the board will work without this setting and so will the driver -> this looks application-specific to me. If there really is a good reason to set TMP006_USE_LOW_POWER and TMP006_USE_RAW_VALUES in board.h then these should be handled like the other TMP006_PARAM* values via tmp006_params.h. But then this change should also be introduced by a seperate PR I think.

MichelRottleuthner · 2018-06-20T09:28:15Z

boards/hamilton/include/board.h

+#define FXOS8700_PARAM_ADDR           (0x1E)
+#define FXOS8700_PARAM_ACC_RANGE      FXOS8700_REG_XYZ_DATA_CFG_FS__8G
+#define FXOS8700_PARAM_RENEW_INTERVAL (1000000ul)
+#define FXOS8700_USE_ACC_RAW_VALUES   (0)


Is this really needed here? FXOS8700_USE_ACC_RAW_VALUES isn't handled by fxos8700_params.h so overwriting this by the application will not be possible. See my other comment regarding tmp006.h

emmanuelsearch · 2018-06-20T09:39:01Z

@Hyungsin @MichelRottleuthner the plan was to test this week and integrate in CI in Berlin with @kaspar030
Stay tuned, we target Friday.

MichelRottleuthner · 2018-06-20T09:46:49Z

Regarding the test application, I do think that providing a complete test application is good for users to experience full features of the board quickly, without tedious trials and errors. This is to mitigate any misunderstanding and hasty frustration.

I agree on that and I like that you kept an eye on usability/simplicity, but at the moment the application doesn't include something specific to this board, does it? Have you tried the saul test appllication? It should do almost the same as this test app with less code.

I'm thinking working on this application further to show that hamilton periodically wakes up, senses all information, and broadcasts a packet including the information. The idle current should be ~6uA.

Then it may be better to issue a PR once the application has this implemented ;)

Edit: maybe the applications repo would be the right place for this specifc app - when placed there you could also add some code to showcase usage of the hamilton cloud service ;)

@emmanuelsearch thanks!

Hyungsin · 2018-06-20T20:24:36Z

boards/hamilton/include/board.h

+#endif
+#ifndef HDC1000_PARAM_RENEW_INTERVAL
+#define HDC1000_PARAM_RENEW_INTERVAL (1000000ul)
+#endif


This part is related to #9386.

Hyungsin · 2018-06-20T22:47:25Z

@MichelRottleuthner, I agree. So I removed the test application and tmp006.h from this PR.

Hyungsin · 2019-03-15T18:26:33Z

Without #10849 merged, you should use two separate terminals.
Type make debug at one terminal and type make term at the other one.
I see the results as below:

Hyungsin · 2019-03-19T18:44:08Z

Rebased

danpetry · 2019-03-20T11:30:24Z

Hi, sorry for the delay! I'll be responsive now, we could try and get this in for the upcoming release perhaps?
I'm getting sporadic behaviour. I have successfully run tests/shell, but then immediately afterwards I flashed and tried to communicate default/examples and the session crashed at the point at which I tried to send commands to the node. By unplugging and plugging back in the segger, I could get it working again, but then the same behaviour happened again. This behaviour is also observed when I flash the applications the other way round.
I ran a debug session in one pane and a RIOT term in another, as described above. Output below

@Hyungsin are you able to recreate, or have any immediate insights into this behaviour?
The auto_init disabling has been here since the tests/ folder was created, see here. @Kijewski or @OlegHahm do you have any recollection of why this was put in?

Hyungsin · 2019-03-21T01:37:11Z

@danpetry, I'm not really sure about this. In my experience, JLink has connection is not always robust... Does turning power off and on before flashing a new app help?

danpetry · 2019-03-21T13:24:45Z

Yes, power cycling of the segger helps (I did this by unplugging and plugging in the usb) but I think we should not require power cycling to re-flash. I just checked with @kaspar030 on his computer/toolchain and he's having similar problems to me. IMO we should solve this so that the support is solid. I'll have a closer look at this and try and isolate the problem. @Hyungsin would you be able to post your Jlink versions and platform (looks like ubuntu 16.04?)

Re the removal of auto_init, initially this was to isolate the test code better. As you point out, now we need it for RTT so should put it back in. It's unlikely to have any adverse effects but let's test on a few other selected boards just in case.

kaspar030 · 2019-03-21T13:39:51Z

Re the removal of auto_init, initially this was to isolate the test code better. As you point out, now we need it for RTT so should put it back in. It's unlikely to have any adverse effects but let's test on a few other selected boards just in case.

I'm just realizing that this would be fine for tests/shell, but may cause trouble in other tests.

Hyungsin · 2019-03-22T01:18:13Z

@danpetry, my Ubuntu version is 16.04, JLink Firmware is J-Link V10, and Hardware is V10.10. Thank you for the endeavor!

Hyungsin · 2019-03-27T18:27:00Z

@danpetry, how is it going? Can we include this PR in the upcoming release?

tcschmidt · 2019-03-27T18:54:43Z

There is still a change request by @kYc0o - has this been resolved?

Hyungsin · 2019-03-27T19:21:35Z

@tcschmidt, yes 👍

tcschmidt · 2019-03-27T23:10:00Z

@tcschmidt, yes 👍

@kYc0o can you confirm?
@danpetry can this be merged then?

danpetry · 2019-03-28T10:41:07Z

This is still not working over this side - we've tested on a number of dev machines to make sure it's not something to do with the setup of mine. @Hyungsin I've got exactly the same OS and Jlink versions as you. Looking into this further.

cladmi · 2019-03-28T11:08:07Z

I added a PR that could at least, show when flashing fails with the board instead on returning 0.
#11303

kYc0o

ACK from my side, though I think the testing is not completely done yet.

cladmi · 2019-03-28T17:09:10Z

Re the removal of auto_init, initially this was to isolate the test code better. As you point out, now we need it for RTT so should put it back in. It's unlikely to have any adverse effects but let's test on a few other selected boards just in case.

I'm just realizing that this would be fine for tests/shell, but may cause trouble in other tests.

I am thinking about something as I currently have a similar issue that when auto_init is disabled I cannot rely on it to have something started.
The issue is that auto_init cannot be enabled by default as one test may not want one init function to be called.
So more the fact that "auto_init" enables all the available "auto_init" functions by default.

It would be a good solution to just disable the auto inclusion of all init functions but still use the requested ones. It would require some refactoring but I think it would still be beneficial when using xfa so could still make sense to do it now.
By changing the #ifdef MODULE_MODULE_NAME to #ifdef MODULE_AUTO_INIT_MODULE_NAME or #ifdef defined(MODULE_AUTO_INIT_MODULE_NAME) && defined(MODULE_MODULE_NAME) and doing the requested definitions in the dependencies handling, auto_init could be split between init which calls the base function and auto_init that enables everything by default.

danpetry · 2019-03-29T08:59:44Z

I've found that the following sequence of operations works reliably, for examples/default, tests/shell, and examples/hello-world.

w1: BOARD=hamilton make flash debug
w2: BOARD=hamilton make term (can get serial output at this point, from w2)
w1: (gdb) c (can give serial input at this point, into w2)

If the second and third steps are swapped, no operation is possible, which is why it wasn't working for me before. I.e. pyterm doesn't seem to connect correctly when the program is actually running. @Hyungsin is this the same on your side?

@cladmi has raised the point that this doesn't allow running automated tests. i.e. make test doesn't work. What's the usual requirement for a new board concerning make test? Is it acceptable to document this and raise an issue? I'm conscious also that this would probably be an extra requirement for @Hyungsin that he wasn't aware of, so I'm happy to help out if we decide it's necessary.

Hyungsin · 2019-03-30T05:00:07Z

@danpetry, @cladmi, alright. Please figure out where we should go.

danpetry · 2019-04-01T15:46:26Z

Here's my assessment of the issues. tl;dr: ACK from me, I'll wait for a day or so before merging in case others want to offer further opinion.

auto_init/xtimer
Stdio_rtt is dependent on xtimer_init() being called, whereas stdio_uart is not, which has caused the issue. tests/shell covers both these modules. Seems to me that enabling auto_init for this test is fine and won't have any adverse effects on the stdio_uart case.

make test
Checking the Ruuvitag and Thingy52, it seems they have the same treatment - i.e. stdio is provided over RTT by default. So, no make test available.

Both of these issues existed before this PR, and the ruuvitag and thingy52 were merged presumably without tests/shell working, and without make test. It's good that this has highlighted some related issues but since they were not caused by the contribution my opinion is that these issues should be discussed and fixed separately. I'll raise the issues.

danpetry · 2019-04-02T12:57:56Z

Comment above updated after some offline discussion with @miri64 . ACK from me and I'll wait before merging for a short while as stated.

Hyungsin · 2019-04-02T17:59:08Z

@danpetry, great! Then should I restore DISABLE=auto_init for the shell test?

danpetry · 2019-04-03T10:12:44Z

@Hyungsin yes, please restore the shell test makefile back to the way it was, squash and rebase. I don't see any blocking comments above so I'll re-test and merge once you've done this :)

Hyungsin · 2019-04-03T22:20:53Z

@danpetry, done!

danpetry · 2019-04-04T10:35:20Z

Re-tested and merged. Thanks for your patience and your contribution! 💯

danpetry · 2019-04-04T12:58:06Z

#11341 adds instructions on RTT shell access for all boards that use it

danpetry · 2019-04-04T14:15:52Z

#11344 and #11343 describe the RTT-related issues with the shell

danpetry · 2019-04-04T14:16:54Z

As described above, these issues were pre-existing and so being raised and dealt with outside the scope of this PR.

Hyungsin force-pushed the forupstream_hamilton branch 7 times, most recently from 1982809 to c080d91 Compare April 24, 2018 19:28

Hyungsin force-pushed the forupstream_hamilton branch from c080d91 to 5f2ccce Compare June 15, 2018 19:49

Hyungsin force-pushed the forupstream_hamilton branch 2 times, most recently from 276c439 to ff4defe Compare June 18, 2018 23:30

Hyungsin force-pushed the forupstream_hamilton branch from ff4defe to 73a0f78 Compare June 18, 2018 23:43

MichelRottleuthner requested changes Jun 19, 2018

View reviewed changes

MichelRottleuthner added Type: new feature The issue requests / The PR implemements a new feature for RIOT Area: boards Area: Board ports labels Jun 19, 2018

Hyungsin force-pushed the forupstream_hamilton branch from 8ce33c8 to 27bf380 Compare June 19, 2018 22:39

MichelRottleuthner requested changes Jun 20, 2018

View reviewed changes

Hyungsin force-pushed the forupstream_hamilton branch 3 times, most recently from f4b62d4 to e29be55 Compare June 20, 2018 20:01

Hyungsin commented Jun 20, 2018

View reviewed changes

Hyungsin force-pushed the forupstream_hamilton branch from e29be55 to d17ff3d Compare June 20, 2018 22:48

Hyungsin mentioned this pull request Mar 15, 2019

drivers: add driver for APDS9007 illuminance sensor #8989

Closed

cladmi mentioned this pull request Mar 28, 2019

jlink.sh: Exit with an error on failure for JLinkExe commands #11303

Merged

kYc0o approved these changes Mar 28, 2019

View reviewed changes

boards: add initial hamilton board support

d9c17c2

danpetry mentioned this pull request Apr 4, 2019

Module initialization - tidyup #11342

Closed

		@@ -0,0 +1,18 @@
		#!/bin/sh

		# This script resets a CC2538SF53 target using JLink called

Conversation

Hyungsin commented Apr 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

References

Uh oh!

Hyungsin commented Jun 15, 2018

Uh oh!

MichelRottleuthner commented Jun 18, 2018

Uh oh!

Hyungsin commented Jun 18, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Hyungsin commented Jun 18, 2018

Uh oh!

MichelRottleuthner 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!

Hyungsin Jun 20, 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!

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MichelRottleuthner commented Jun 19, 2018

Uh oh!

Hyungsin commented Jun 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emmanuelsearch commented Jun 20, 2018

Uh oh!

Hyungsin commented Apr 24, 2018 •

edited

Loading

Hyungsin commented Jun 18, 2018 •

edited

Loading

Hyungsin Jun 20, 2018 •

edited

Loading

Hyungsin commented Jun 19, 2018 •

edited

Loading

MichelRottleuthner commented Jun 20, 2018 •

edited

Loading

Hyungsin commented Mar 15, 2019 •

edited

Loading

danpetry commented Mar 20, 2019 •

edited

Loading

Hyungsin commented Mar 22, 2019 •

edited

Loading