boards/msbiot: Configured cc110x as default netdev by maribu · Pull Request #9124 · RIOT-OS/RIOT

maribu · 2018-05-12T11:46:14Z

Contribution description

boards/msbiot/Makefile.include:

Use module "cc110x" if netdev_default or gnrc_netdev_default
is used

boards/msbiot/include/cc110x_params.h:

Added configuration for the CC1101 transceiver

Issues/PRs references

aabadie

Thanks for working on this. I think the default parameters management for the cc110x could be better improved than what is proposed here.
See my comment below.

aabadie · 2018-05-12T15:41:30Z

boards/msbiot/Makefile.include

+
+# add driver for CC1101 sub-gigahertz transceiver as default netdev
+ifneq (,$(filter netdev_default gnrc_netdev_default,$(USEMODULE)))
+	USEMODULE += cc110x


indent should be only 2 spaces

Thanks for the hint. I will fix this. Is there a reason why RIOT uses 4 spaces in C code, but 2 in Makefiles? This seems inconsistent.

I personally use smart tabs 1, 2 for just this reason: You don't need to argue about indent width, because it because an editor option everyone can configure to her/his liking. And if needed, to different width on a per file type bases.

aabadie · 2018-05-12T15:45:09Z

boards/msbiot/include/cc110x_params.h

@@ -0,0 +1,46 @@
+/*


A better solution would be to move this file in drivers/cc110x/include then using #ifndef/#define/#endif it will be possible to set default values in the driver code that can be overridden in the boards. The msba2 could also be updated.
See how it's done with at86rf2xx driver and samr21-xpro/iotlab-m3 boards.

Thanks for the hint. I will update it accordingly.

Btw: Does the cc110x work for those boards? Last time I checked the driver didn't work with either msba2 or msbiot. The driver was developed when spi_acquire was a rather cheap operation of acquiring a mutex, not setting up the SPI bus. Before the SPI API was changed so that spi_acquire also configured the SPI bus, the driver worked on both msba2 and msbiot. So optimizing the driver to call spi_acquire only once at the very beginning of every interaction with the hardware and spi_release at the very end might not only improve performance, but also fix the issue with msba2 and msbiot boards.

maribu · 2018-05-12T19:07:17Z

@aabadie: Thanks for pointing out how to improve the pull request. I believe I have addressed the issues you mentioned now.

aabadie

Thanks for updating the PR. It's almost there but I still have a few comments. See below.

aabadie · 2018-05-14T06:32:52Z

drivers/cc110x/include/cc110x_params.h

+ * These values are based on the msbiot board
+ * @{
+ */
+#ifndef CC110X_SPI


I would call this parameter CC110X_PARAM_SPI, same for other parameters (add _PARAM after CC110X).

aabadie · 2018-05-14T06:34:41Z

drivers/cc110x/include/cc110x_params.h

+ */
+const cc110x_params_t cc110x_params[] = {
+    {
+        .spi  = CC110X_SPI,  ///< SPI bus the cc110x tranceiver is connected to


No need for adding doxygen documentation. This struct is already "documented" in the cc110x.h file (I looked at it and it should be updated in a follow PR).

aabadie · 2018-05-14T06:36:28Z

drivers/cc110x/include/cc110x_params.h

+#ifndef CC110X_GDO2
+#define CC110X_GDO2                GPIO_PIN(PORT_C,  5)
+#endif
+/** @} */


Please also add a CC110X_PARAMS define, like it's done in at86rf2xx driver. This way it allows a developer to define a global params define for multiple devices. With your change, this is not possible.

aabadie · 2018-05-14T06:37:03Z

boards/msba2/include/board.h

+ * @name    Configure conntect CC1101 (radio) device)
+ * @{
+ */
+#define CC110X_SPI          0


Use SPI_DEV(0)

aabadie · 2018-05-14T06:38:33Z

boards/msba2/include/board.h

+ * @{
+ */
+#define CC110X_SPI          0
+#define CC110X_CS           53


Those values are weird, is it possible to find something with GPIO_PIN(PORT_NUM, PIN_NUM) ? I know it's out of the scope of this PR, I'm just commenting

In cpu/lpc2387/include/periph_cpu.h:57 I found this

#define GPIO_PIN(port, pin) (port*32 + pin)

So I guess it should be like this

CS = 53 = GPIO_PIN(PORT_B, 21) GDO0 = 27 = GPIO_PIN(PORT_A, 27) GDO1 = 55 = GPIO_PIN(PORT_B, 23) GDO2 = 28 = GPIO_PIN(PORT_A, 28)

I will check this tomorrow on the hardware. Would be nice to have no magic numbers in the code.

No need to test, this will obviously yield the same value and thus has to be correct.

I think this is better:

CS = 53 = GPIO_PIN(1, 21) GDO0 = 27 = GPIO_PIN(0, 27) GDO1 = 55 = GPIO_PIN(1, 23) GDO2 = 28 = GPIO_PIN(0, 28)

This would be consistent with the rest of the MSBA2 board.h and the pin labels at the board diagram.

maribu · 2018-05-14T20:24:48Z

@aabadie: Thanks for your input. Please re-review. I think I addressed all issues now.

aabadie · 2018-05-14T20:44:44Z

@maribu, I allowed myself to push an extra commit in your branch that changes your PR to what I have in mind:

the default params of cc110x uses the values for msba2, since it was already the case before
only board.h in msbiot needs to have the param macro defined

What would be great now is that you squash your commits in 2:

one related to the changes in msba2 and the move of the params.h file to drivers
one related to the msbiot board change

Then it will be perfect. Maybe someone else could also test that the msba2 board is still working (I don't have one).

maribu · 2018-05-15T12:27:39Z

@aabadie: Done.

The reviewing will sadly be a bit problematic:

I'm 100% sure that all existing msbiot boards are in my office
There aren't too many MSBA2 boards as well - maybe our work group is also the proud owner of all remaining and operational boards?
The cc110x driver is known to not work since more than a year. So actually checking that this PR doesn't break anything requires the fix first
- Hopefully I'll have some time to work on that soon

maribu · 2018-05-15T15:12:54Z

I was wrong, sort of:

--- fe80::ff:fe00:51 ping statistics ---
100 packets transmitted, 10 received, 90% packet loss, time 189.06358086 s
rtt min/avg/max = 16.621/1.663/16.650 ms

Above when using the original pull request (without moving cc110x_params.h to the driver) over a distance of ~10cm.

main(): This is RIOT! (Version: 2018.07-devel-195-g33116-faultier2go)
gcoap example app
All up, running the shell now
> cc110x: initialized with address=81 and channel=0
[CC110X spi] fatal error

With the current PR. Over SWD I get the following backtrace:

#1  0x08000912 in _shoot (timer=<optimized out>) at /home/maribu/Repos/software/RIOT/sys/xtimer/xtimer_core.c:159

I do believe that the PR does not contain the bug that causes the fatal error, but merely triggers it. But still it might be reasonable to not merge the PR until the cc110x issue is resolved

aabadie · 2018-05-15T15:14:11Z

But still it might be reasonable to not merge the PR until the cc110x issue is resolved

+1

maribu · 2018-05-15T17:47:28Z

Of course it was an evil typo. After having that fixed, it works again (at least as "good" as before, with 90% loss rate).

I will check for the MSBA2 tomorrow as well. But I believe the MSBA2 is not even booting since the update of the SPI API.

maribu · 2018-05-19T09:45:35Z

Hi,

I solved the issue with the MSBA2 not booting with the cc110x driver in PR #9154. Once PR #9154 is applied, the cc110x driver works with and without this PR. So I can confirm, that this PR does not break thinks.

Below is the output of one of our MSBA2 boards after applying this PR and PR #9154 on top:

2018-05-19 11:40:51,393 - INFO # Connect to serial port /dev/ttyUSB0
Welcome to pyterm!
Type '/exit' to exit.
2018-05-19 11:40:52,401 - INFO # random: NO SEED AVAILABLE!
2018-05-19 11:40:52,407 - INFO # cc110x: initialized with address=34 and channel=0
2018-05-19 11:40:52,414 - INFO # main(): This is RIOT! (Version: 2018.07-devel-224-g30a4e-faultier2go-msbiot)
2018-05-19 11:40:52,417 - INFO # gcoap example app
2018-05-19 11:40:52,420 - INFO # All up, running the shell now
> 2018-05-19 11:40:52,425 - INFO #  ping6 fe80::ff:fe00:22
2018-05-19 11:40:52,433 - INFO # 12 bytes from fe80::ff:fe00:22: id=83 seq=1 hop limit=64 time = 0.351 ms
2018-05-19 11:40:53,431 - INFO # 12 bytes from fe80::ff:fe00:22: id=83 seq=2 hop limit=64 time = 0.351 ms
2018-05-19 11:40:54,468 - INFO # 12 bytes from fe80::ff:fe00:22: id=83 seq=3 hop limit=64 time = 0.350 ms
2018-05-19 11:40:54,471 - INFO # --- fe80::ff:fe00:22 ping statistics ---
2018-05-19 11:40:54,477 - INFO # 3 packets transmitted, 3 received, 0% packet loss, time 2.0620426 s
2018-05-19 11:40:54,481 - INFO # rtt min/avg/max = 0.350/0.350/0.351 ms

Kind regards,
Marian

aabadie

#9154 is merged. @maribu Can you rebase and re-check on both msba2 and msbiot boards ?

I also have a remaining minor doxygen comment, see below.

After that we can merge.

aabadie · 2018-05-21T08:07:27Z

drivers/cc110x/include/cc110x_params.h

+
+
+/**
+ * @name    CC110X configuration


Missed that one. Here it should be @brief and the @{, /** @} */ below can be removed.

- Replaced magic numbers in the CC110X configuration of the MSBA2 with SPI_DEV(x) and GPIO_PIN(x, y) macros - Adjusted implementation of `cc110x_params.h` to match the code of `at86rf2xx` - Made MSBA2's CC110X parameters the default configuration

- Adjusted `#define`s to match the naming convention of `cc110x_params.h` - Enable the `cc110x` module if `netdev_default` or `gnrc_netdev_default` is used

maribu · 2018-05-21T21:12:53Z

Hi,
I can confirm that the cc110x still works fine on the MSBA2 after this PR. For the MSB IoT it still works as good as before (90% loss rate at ping).

Regarding the MSB IoT: I have figured out some timing issues in the driver. The MSB IoT seems to be faster and enables interrupts on the GDO2 control pin before the CC1101 transceiver applies the new configuration. Because the interrupt triggers on both rising and falling edge, it will can off two times: Let's say the GDO2 signals by falling edge that more data can be uploaded to the TX fifo but the GDO2 was low before, it would have to go high first and later low again. Thus, only the second edge should trigger an interrupt, but the MSB IoT might run the ISR two times. Sadly those timing issues aren't the only problem. It seems the SPI transfer to the MSB IoT is not working completely reliable. I hope to figure that out as well. (It might have to do with how the CS is done.) Maybe I can solve that next weekend.

Kind regards,
Marian

aabadie

All good now, ACK

aabadie requested changes May 12, 2018

View reviewed changes

aabadie requested changes May 14, 2018

View reviewed changes

maribu force-pushed the msbiot branch 4 times, most recently from 46fbbf4 to e671f28 Compare May 14, 2018 20:22

maribu force-pushed the msbiot branch from 1745a23 to e7edec0 Compare May 15, 2018 12:16

aabadie added Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation Area: drivers Area: Device drivers CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels May 15, 2018

maribu force-pushed the msbiot branch from e7edec0 to 9b122f7 Compare May 15, 2018 17:44

maribu force-pushed the msbiot branch 2 times, most recently from 13821e6 to 30a4e41 Compare May 19, 2018 09:30

aabadie reviewed May 21, 2018

View reviewed changes

maribu force-pushed the msbiot branch from 30a4e41 to 524086b Compare May 21, 2018 19:22

maribu added 2 commits May 21, 2018 22:59

boards/msbiot: Added/updated configuration for the cc110x transceiver

d97340c

- Adjusted `#define`s to match the naming convention of `cc110x_params.h` - Enable the `cc110x` module if `netdev_default` or `gnrc_netdev_default` is used

maribu force-pushed the msbiot branch from 524086b to d97340c Compare May 21, 2018 21:00

aabadie approved these changes May 22, 2018

View reviewed changes

aabadie merged commit 912ef32 into RIOT-OS:master May 22, 2018

maribu deleted the msbiot branch May 22, 2018 20:12

aabadie mentioned this pull request May 25, 2018

boards+cc110x: introduce cc110x default config #8437

Closed

PeterKietzmann mentioned this pull request May 29, 2018

boards: define default radio for wsn430 boards #8438

Merged

cladmi added this to the Release 2018.07 milestone Jul 10, 2018

Conversation

maribu commented May 12, 2018 • edited by aabadie 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!

maribu commented May 12, 2018

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!

maribu commented May 14, 2018

Uh oh!

aabadie commented May 14, 2018

Uh oh!

maribu commented May 15, 2018

Uh oh!

maribu commented May 15, 2018

Uh oh!

aabadie commented May 15, 2018

Uh oh!

maribu commented May 15, 2018

Uh oh!

maribu commented May 19, 2018

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!

maribu commented May 21, 2018

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

maribu commented May 12, 2018 •

edited by aabadie

Loading