Jiminy Support for hwrng and at86rfr2 transceiver by Josar · Pull Request #8742 · RIOT-OS/RIOT

Josar · 2018-03-05T19:05:10Z

Support for hwrng
Included at86rfr2 support into at86rf2xx driver.

jnohlgard · 2018-03-05T20:31:18Z

Could you open a separate PR for the atmega stack pointer commit? It will make review easier and keep this PR focused on one task.

Josar · 2018-03-06T08:41:26Z

@gebart done

Josar · 2018-03-09T01:06:52Z

@PeterKietzmann maybe this is also interresting for you as you have the hardware at hand. ;-)

Josar · 2018-03-27T18:18:16Z

@gebart and @PeterKietzmann just curious if someone has time to review this?

PeterKietzmann · 2018-03-27T19:51:14Z

@Josar, currently I don't have time to provide a proper review. I do my best to come back to this soon, but I can't promise anything.

jnohlgard

There are some unrelated changes in here (code style, whitespace fixes etc) which I think is fine to have in the PR, but they should be in a separate commit before all of the rfr2 additions. There are also a few points in the rfr2 driver where I have some comments, see inline comments below

jnohlgard · 2018-03-27T18:32:46Z

cpu/atmega256rfr2/Makefile.features


 # Various other features (if any)
 FEATURES_PROVIDED += periph_cpuid
+FEATURES_PROVIDED += periph_hwrng


Missing newline at end of file

jnohlgard · 2018-03-27T18:37:54Z

drivers/at86rf2xx/at86rf2xx.c

+#include "od.h"
+#endif
+
+#ifdef MODULE_AT86RFR2


Maybe this whole block could go in its own file at86rfr2_isr or something? It's the only CPU dependent functionality in the driver so it makes sense to move it out of the generic parts.

jnohlgard · 2018-03-27T18:42:58Z

cpu/atmega256rfr2/periph/hwrng.c

+
+    /* Manual p. 119 radio transceiver in Basic Operating Mode receive state*/
+    while (TRX_STATUS != 0x06) {
+        TRX_STATE = 0x06;


I am worried that this will lead to a race between the radio driver and the hwrng driver

Could you please eleborate this?

Imagine this scenario:

two threads: a low priority thread that uses the hwrng, a high priority thread that reacts to some outside event (e.g. gpio interrupt, or timer)

hwrng thread is running and using the hwrng

outside event occurs, low priority thread is pre-empted in favor of the high priority thread

high priority thread wants to interact with the radio in some way (e.g. Send packet or put the radio to sleep)

hwrng has left the transceiver in an unexpected state. <- possible problem

high priority thread finishes

hwrng restores a state which was saved before, possibly overwriting what the high priority thread did. <- Possible problem.

jnohlgard · 2018-03-27T18:45:20Z

cpu/atmega256rfr2/periph/hwrng.c

+            regVal = regVal >> 5;
+            regVal = regVal << 2 * i;
+            rnd |= regVal;
+            xtimer_usleep(1);


Is this sleep necessary to the hardware timing? It is short enough to be better suited for xtimer_spin.

jnohlgard · 2018-03-27T18:47:41Z

drivers/at86rf2xx/at86rf2xx.c

+
+#ifdef MODULE_AT86RFR2
+    /* Store device pointer for interrupts */
+    static_dev = (netdev_t *)dev;


I assume the at86rfr2 only ever has one transceiver in the same CPU

It is a SOC and will ever features only one transceiver.
You could also add an additional external transceiver, but this is also not featured with the current implementation. And i do not see why someone would do this.

I guess this would fall under #4876. The static_dev variable is fine since there is only one instance of the rfr2

jnohlgard · 2018-03-27T19:07:01Z

drivers/at86rf2xx/at86rf2xx.c

    at86rf2xx_reg_write(dev, AT86RF2XX_REG__IRQ_MASK,
                        AT86RF2XX_IRQ_STATUS_MASK__TRX_END);
-    /* clear interrupt flags */
-    at86rf2xx_reg_read(dev, AT86RF2XX_REG__IRQ_STATUS);


Unrelated change, please split into its own commit

jnohlgard · 2018-03-27T19:23:27Z

drivers/at86rf2xx/at86rf2xx.c

+//        dev->idle_state = state;
+//    }
+
+    while (state != AT86RF2XX_STATE_TX_ARET_ON) {


This change is not obvious what it fixes, what was broken before?
It should probably go in a separate commit.

jnohlgard · 2018-03-27T19:50:47Z

drivers/Makefile.dep

  USEMODULE += netif
  USEMODULE += ieee802154
  USEMODULE += netdev_ieee802154
  FEATURES_REQUIRED += periph_gpio


Does the rfr2 need gpio?

jnohlgard · 2018-03-27T19:52:35Z

drivers/at86rf2xx/at86rf2xx_internal.c

+
+#ifdef MODULE_AT86RFR2
+/* SOC uses no SPI, but uses memcpy */
+    #include <string.h>


These should not be indented

jnohlgard · 2018-03-27T19:56:50Z

drivers/at86rf2xx/at86rf2xx_internal.c

+#ifdef MODULE_AT86RFR2
+
+    /* check if frame buffer protection is active, rx frame was not read jet*/
+/*  if( ( *(AT86RF2XX_REG__TRX_CTRL_2) &(1<<RX_SAFE_MODE)) != 0)


jnohlgard · 2018-03-27T20:08:51Z

One more question: does the hwrng driver stand on its own or does it require the at86rf2xx driver to be initialized beforehand?
If it does, then the dependencies need to reflect that in one way or the other.

Josar · 2018-03-28T00:37:41Z

The hwrng is intentional not dependent on the at86rf2xx Module. This maybe lead to a little bit of overhead but i think this is okay.

jnohlgard · 2018-03-28T05:13:18Z

Is there any risk that initialization of the hwrng driver will mess up the state of the at86rf2xx driver? They are touching the same hardware after all

Josar · 2018-03-28T08:59:51Z

Yes, there is. And for that all states are saved and set back to previous states.

Reading exactly what you wrote, NO, there is nothing done in initialization.

Everything is done when reading some random numbers and set back afterwards.

Josar · 2018-04-19T19:24:24Z

@gebart tried to addressed your requests. Had to rebase as evtimer changes had to be applied for proper function.

Josar · 2018-05-08T13:52:28Z

@gebart and @PeterKietzmann maybe you could find some time to review this?
Rebased as there where some bigger changes in the atmegas.

jnohlgard

hwrng: Some code style issues exist, and I am not sure that xtimer is necessary in the current implementation.
Did not have time to review the radio changes

jnohlgard · 2018-05-21T13:08:51Z

cpu/atmega256rfr2/periph/hwrng.c

+        uint8_t rnd = 0;
+        for (uint8_t i = 0; i < 4; ++i) {
+            /* Random value update period 1us p. 47*/
+            xtimer_spin(wait);


Is this necessary?
I imagine that the CPU instructions leading up to this point take at least 1 µs on atmegas?

jnohlgard · 2018-05-21T13:10:01Z

cpu/atmega256rfr2/periph/hwrng.c

+    transition();
+
+    /* Manual p. 119 radio transceiver has to be in Basic Operating Mode receive state*/
+    if ( (TRX_STATUS&0x1f)!= 0x06) {


this 0x1f magic number is used in several places in this file, it should be converted to a define, or if possible, reuse the hardware register bit masks from the radio driver.

jnohlgard · 2018-05-21T13:13:40Z

cpu/atmega256rfr2/periph/hwrng.c

+    RX_SYN &= ~(1 << RX_PDT_DIS);
+
+    /* Manual p. 105 RPC/SRT has to be disabled. */
+    TRX_RPC &= ~(1<<RX_RPC_CTRL1 | 1<<RX_RPC_CTRL0);


uncrustify should be able to fix code style spacing and indentation issues in this file. There is a configuration in the RIOT tree root (uncrustify-riot.cfg)

Josar · 2018-05-23T20:02:27Z

Closed in favor of #9172

@gebart thread concurrency has to be solved in future releases so i will skip hwrng in favor of the transciever.
#8742 (comment)

Josar force-pushed the jiminy_master branch from 5f13775 to 2862024 Compare March 5, 2018 20:42

Josar force-pushed the jiminy_master branch 2 times, most recently from 16379c3 to 35917ee Compare March 9, 2018 01:04

Josar force-pushed the jiminy_master branch from 35917ee to 830fe18 Compare March 20, 2018 13:44

PeterKietzmann requested review from PeterKietzmann and jnohlgard March 27, 2018 19:51

PeterKietzmann self-assigned this Mar 27, 2018

PeterKietzmann added Area: drivers Area: Device drivers Area: boards Area: Board ports labels Mar 27, 2018

jnohlgard requested changes Mar 27, 2018

View reviewed changes

Josar force-pushed the jiminy_master branch from 830fe18 to 0d6f0aa Compare March 28, 2018 02:43

Josar force-pushed the jiminy_master branch 4 times, most recently from 81086d4 to 05e1494 Compare March 28, 2018 16:38

Josar mentioned this pull request Mar 29, 2018

Race condition RIOT and Radvd #8850

Closed

Josar force-pushed the jiminy_master branch 5 times, most recently from 5306d1a to f60ee90 Compare April 19, 2018 19:20

Josar force-pushed the jiminy_master branch from f60ee90 to 20f0290 Compare April 20, 2018 02:05

Josar force-pushed the jiminy_master branch from 20f0290 to 3813c1f Compare May 8, 2018 13:49

Josar force-pushed the jiminy_master branch from 3813c1f to 76e8c49 Compare May 8, 2018 14:02

Josar added 4 commits May 15, 2018 18:54

drivers/../at86rf2xx.c: reorder code

d978ed0

cpu/atmega256rfr2: hwrng support

0f9468e

tests/periph_hwrng: resolve integer overflow

5d7b010

drivers/at86rf2xx: atmega256rfr2 support

7c44323

Josar force-pushed the jiminy_master branch from 76e8c49 to 7c44323 Compare May 15, 2018 17:01

jnohlgard requested changes May 21, 2018

View reviewed changes

Josar mentioned this pull request May 23, 2018

Jiminy Support for at86rfr2 transceiver #9172

Closed

Josar closed this May 23, 2018

Conversation

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

Uh oh!

Uh oh!

jnohlgard commented Mar 5, 2018

Uh oh!

Josar commented Mar 6, 2018

Uh oh!

Josar commented Mar 9, 2018

Uh oh!

Josar commented Mar 27, 2018

Uh oh!

PeterKietzmann commented Mar 27, 2018

Uh oh!

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

jnohlgard commented Mar 27, 2018

Uh oh!

Josar commented Mar 28, 2018

Uh oh!

jnohlgard commented Mar 28, 2018

Uh oh!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

Josar commented May 8, 2018

Uh oh!

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

Josar commented May 23, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Josar commented Mar 5, 2018 •

edited

Loading

Josar commented Mar 28, 2018 •

edited

Loading

Josar commented Apr 19, 2018 •

edited

Loading