drivers/mrf24j40: Add mrf24j40 radio support #6277

bergzand · 2016-12-31T13:22:32Z

This patch collection adds support for the mrf24j40 radio from microchip.

supported:

transmit/receive
most of netdev2 get/set (ack_req, csma setting...)
sleep/reset

not supported:

retransmission settings (not supported by the radio)
off state (not supported by the radio)
preloading (hangs the driver, did not investigate any further)

As visible in the commits, this PR builds on the initial work of @Nenaco, but the whole driver logic is rewritten to prevent a deadlock. I hope I've fixed most of the comments from the original PR ( #5792).

OlegHahm · 2017-01-03T11:02:42Z

Just a site note: apparently the mail address you're using for your git commits seems not to be added to your GitHub profile.

bergzand · 2017-01-03T18:04:31Z

Thanks for the notice, should be fixed now

bergzand · 2017-01-08T14:57:44Z

Whoops, something went wrong there

bergzand · 2017-01-08T15:05:38Z

And some git stupidity of my own.

Driver should be uncrustified and static check gives green on everything except a PR squash message

OlegHahm · 2017-01-14T10:20:15Z

@PeterKietzmann, you have the hardware in Hamburg, right? Any candidate for review and testing?

PeterKietzmann · 2017-01-14T10:29:50Z

Yes I have and Hauke should as well. The candidate is me but I can't promise to have an in-detail review and test until next Friday (feature freeze)

Nenaco · 2017-01-15T16:04:52Z

Hi Folks,
thanks for this new PR on the topic MRF24J40 driver development. I'm very sad that I had no time to drive the PR #5792 further on, but there was so much pressure on my person so that my engagement stopped for some weeks. I have only seen in the news letter, that there's something going on about the MRF24J40 in the background. Great!
Regards,
Neo

haukepetersen · 2017-01-18T22:09:54Z

Yes, I also do have 2 of these somewhere in my drawer. So do I get this right, that this PR is an alternative (or even successor) of #5792?

PeterKietzmann · 2017-01-18T22:13:27Z

It's a successor. Wanted to give it a quick try tomorrow but as this needs adoptions to the SPI rework I assume it might be too late for this release. But the next hack'n'ack is coming and this driver has been on my todo list for so long.

haukepetersen

Some minor style issues and simplifications

haukepetersen · 2017-01-18T22:10:24Z

drivers/Makefile.dep

@@ -18,6 +18,19 @@ ifneq (,$(filter at86rf2%,$(USEMODULE)))
  endif
 endif

+ifneq (,$(filter mrf24j40,$(USEMODULE)))
+  USEMODULE += mrf24j40


this line strikes me as quite useless...

I think that one is directly copied from the at86rf2% above it. While it makes sense there, it looks indeed useless here.

haukepetersen · 2017-01-18T22:14:19Z

drivers/include/mrf24j40.h

+/**
+ * @brief   Maximum possible packet size in byte
+ */
+#define MRF24J40_MAX_PKT_LENGTH        (IEEE802154_FRAME_LEN_MAX)


Why redefine IEEE802154_FRAME_LEN_MAX here?

haukepetersen · 2017-01-18T22:40:53Z

drivers/include/mrf24j40.h

+ */
+#define MRF24J40_DEFAULT_ADDR_SHORT    (0x0230)
+#define MRF24J40_DEFAULT_ADDR_LONG     (0x1222334455667788)
+/** @} */


Please use the newly (to be) introduced uuid module for address generation -> #6314

haukepetersen · 2017-01-18T22:43:39Z

drivers/include/mrf24j40.h

+#define MRF24J40_MIN_CHANNEL           (IEEE802154_CHANNEL_MIN)
+#define MRF24J40_MAX_CHANNEL           (IEEE802154_CHANNEL_MAX)
+#define MRF24J40_DEFAULT_CHANNEL       (IEEE802154_DEFAULT_CHANNEL)
+/** @} */


again, IMHO no need to redefine, just use the IEEE802154 defines directly

haukepetersen · 2017-01-18T22:46:20Z

drivers/mrf24j40/include/mrf24j40_params.h

+#endif
+
+#define MRF24J40_PARAMS_DEFAULT    { .spi = MRF24J40_PARAM_SPI, \
+                                     .spi_speed = MRF24J40_PARAM_SPI_SPEED, \


indention one off

haukepetersen · 2017-01-18T22:50:28Z

tests/driver_mrf24j40/Makefile

@@ -0,0 +1,16 @@
+APPLICATION = driver_mrf24j40


Though I appreciate the effort put into this test application, I tend to say we can savely drop it. Testing the driver would simply be:

build the examples/gnrc_networking with USEMODULE=mrf24j40 BOARD=BLBLBLBL make

set/get options (addresses, channel, pan)

send packets

done

Or did I miss any additional functionality provided by this test app?

It is basically the same driver test as the driver_at86rf2xx test (so, not much effort). Since (I believe) the gnrc_networking example also has a test on the link layer (the function called txtsnd) I agree.

Or did I miss any additional functionality provided by this test app?

Automated testing?

Since (I believe) the gnrc_networking example also has a test on the link layer (the function called txtsnd) I agree.

#6397 removed that and fixed #5034 through that.

(also, if all other drivers are able to send via IP and not via mrf24j40 it is safe to assume that device is at fault ;-))

So this test should be removed then?

Yes. Please remove it. One can use gnrc_networking or default. Later #5286 will unify these tests anyway.

bergzand · 2017-01-19T09:09:28Z

I fixed the first set of issues.
I can rework the driver to use the new uuid framework and the new SPI api if you really want this driver in the new release. I should have time to rebase it on those patches this evening or tomorrow morning (UTC+1).
Otherwise I'm going to wait until those are merged before incorporating those changes in the driver

PeterKietzmann · 2017-01-19T09:13:13Z

@bergzand that would be awesome in any way! But to have a realistic idea if this would make it into the release I will try to connect one until the afternoon

PeterKietzmann · 2017-01-20T09:53:31Z

@bergzand sorry I didn't make it yesterday but now I gave it a shot and I was pretty happy to see the state of the driver. I used gnrc_networking and "palyed around" a bit. Setting parameters (number of retransmissions is not adjustable?) as well as hardcore pinging of small and big fragmented IPv6 packets between this device and an Atmel with at86rf2xx radio. Now we can eiter hurry up very much and try to get this in until the afternoon to let this driver be part of the 2016.01 release. But the uuid is already merged and the SPI PR has priority. Or we can give it just a bit more time and handle this calmly.

bergzand · 2017-01-20T10:50:32Z

@PeterKietzmann Thank you for testing the driver.

@bergzand sorry I didn't make it yesterday but now I gave it a shot and I was pretty happy to see the state of the driver. I used gnrc_networking and "palyed around" a bit. Setting parameters (number of retransmissions is not adjustable?) as well as hardcore pinging of small and big fragmented IPv6 packets between this device and an Atmel with at86rf2xx radio.

The number of retransmissions is unfortunately not configurable. Blame Microchip for that one.

Now we can eiter hurry up very much and try to get this in until the afternoon to let this driver be part of the 2016.01 release. But the uuid is already merged and the SPI PR has priority. Or we can give it just a bit more time and handle this calmly.

I just rebased the driver and added another commit applying the uuid implementations. Besides my node having a different (static) IPv6 address now, everything seems to work.
The minor style issues commit is amended to also use the IEEE802154_DEFAULT_PANID, seems that I missed that one. Furthermore, I corrected the initial commit of @Nenaco to have him as an author again.

I should have some time today to apply the SPI API change, but your call if you want to hurry things up or test a bit more before merging

bergzand · 2017-01-20T13:22:47Z

Test removed

PeterKietzmann

@bergzand I added a bunch of comments. Mostly concerning style issues, probably not even introduced by you. However, now that you shepherd this, please adapt them. In addition it would be great if you scan over the code yourself because I didn't repeat to comment same errors multiple times.
Most likely we will postpone the feature freeze until Monday. In that case there is still some time to adapt this PR.
I will do further tests with this PR but I have a good feeling about it in general.
Before making new commits I propose to squash.

PeterKietzmann · 2017-01-20T13:27:23Z

drivers/include/mrf24j40.h

+ * @brief       Interface definition for MRF24J40 based drivers
+ *
+ * @author      neo@nenaco.de
+ * @author      koen@bergzand.net


The copyright above indicates that @TobiasFredersdorf was the (initial?) author of this file. Please make the copyright and authors clear. The authors line should include a name, -if you insist to stay anonymous take an alias- and a valid mail address

PeterKietzmann · 2017-01-20T13:29:30Z

drivers/include/mrf24j40.h

+
+/**
+ * @defgroup    drivers_mrf24j40 MRF24J40 based drivers
+ * @ingroup     drivers_netdev


Please use this group: drivers_netdev_netdev2

PeterKietzmann · 2017-01-20T13:30:22Z

drivers/include/mrf24j40.h

+ * @author      koen@bergzand.net
+ */
+
+#ifndef MRF24J40_H_


Here and everywhere else, please remove the trailing underscores as done in #6407

PeterKietzmann · 2017-01-20T13:35:04Z

drivers/include/mrf24j40.h

+ *
+ * @return                  Number of retransmissions (3)
+ */
+uint8_t mrf24j40_get_max_retries(mrf24j40_t *dev);


Seems redundant as this parameter is not adjustable. But I won't stall this PR for that reason.

PeterKietzmann · 2017-01-20T13:37:08Z

drivers/include/mrf24j40.h

+ *
+ * @param[in] dev           device to read from
+ *
+ * @return                  the currently set (2-byte) short address


All functions below do not return errors or are not documented properly

PeterKietzmann · 2017-01-20T14:56:09Z

drivers/mrf24j40/mrf24j40_netdev.c

+                netdev->event_callback(netdev, NETDEV2_EVENT_TX_COMPLETE);
+            }
+        }
+#endif


I assume the whole block from 546:566 is not only relevant for netstats?

I assumed it was irrelevant since in sys/net/gnrc/link_layer/netdev2/gnrc_netdev2.c the switch statement also has the same ifdef around the handlers of those callback options (sys/net/gnrc/link_layer/netdev2/gnrc_netdev2.c 76:83. If you want the ifdef removed or minimized, I'm okay with that, but as it is now, it saves an SPI transfer when L2 stats are not enabled and I would like to keep that

@bergzand thanks for the explanation. Leave it as is for now. Probably I will come back to you on that point.

PeterKietzmann · 2017-01-20T14:56:21Z

drivers/mrf24j40/mrf24j40_netdev.c

+            netdev->event_callback(netdev, NETDEV2_EVENT_RX_COMPLETE);
+        }
+        dev->pending &= ~(MRF24J40_TASK_RX_READY);
+    } // end of RXIF check


Comment style

PeterKietzmann · 2017-01-20T14:56:32Z

drivers/mrf24j40/mrf24j40_netdev.c

+#endif
+
+    }
+    /* RECEIVE INTERRUPT OCCURED */


why capitaly?

PeterKietzmann · 2017-01-20T14:57:34Z

sys/auto_init/netif/auto_init_mrf24j40.c

+ * @file
+ * @brief   Auto initialization for mrf24j40 network interfaces
+ *
+ * @author  <neo@nenaco.de>


Copyright and author?

PeterKietzmann · 2017-01-20T14:58:22Z

sys/auto_init/netif/auto_init_mrf24j40.c

+ * @{
+ */
+#define MRF24J40_MAC_STACKSIZE     (THREAD_STACKSIZE_DEFAULT)
+#define MRF24J40_MAC_PRIO          (THREAD_PRIORITY_MAIN - 4)


please use

#ifndef MRF24J40_MAC_PRIO #define MRF24J40_MAC_PRIO (GNRC_NETDEV2_MAC_PRIO) #endif

PeterKietzmann · 2017-01-24T07:51:11Z

@bergzand that's great. Thanks a lot! Now we 'only' have to wait for SPI but this one is just the next in the queue.

bergzand · 2017-01-25T21:11:16Z

Rebased after the merge of #4780

PeterKietzmann · 2017-01-25T21:12:23Z

@bergzand wow that was fast :-)! I will give it a last try tomorrow (don't have the hardware with me). @haukepetersen please dismiss your change request. Has been addressed.

bergzand · 2017-01-25T22:51:34Z

I noticed the SPI API merge fast due to an email notification, and a rebase+push is not that much work. :-)

I've added a check to the initialization function so that when debugging is enabled in /drivers/mrf24j40/mrf24j40_internal.c a register is checked and written to and checked again to verify SPI communication with the mrf24j40. This because we both sometimes seem to have trouble with the SPI communication causing RIOT to hang during initialization without any helpful debug messages.

PeterKietzmann · 2017-01-26T08:33:49Z

I can't get it running any more. No time to debug now but it must be that the program hangs before main() starts. Besides of that, please squash you commits to one. The first commit message looks fine.

PeterKietzmann · 2017-01-26T08:56:32Z

BTW: It is -as you expected- that the SPI communication during mrf24j40_init fails

bergzand · 2017-01-26T09:16:48Z

Squashed.

@PeterKietzmann With my current setup (nucleo-f401 with an mrf24j40), I have to call periph_init() at the end of the board_init() function in board.c. ~~I'm not sure if this is due to the new SPI API or due to the fact that I power the mrf24j40 via a GPIO pin of the nucleo (Yes, I know, it's ugly, but it works).~~
~~With the the mrf24j40 on a regular power pin, the same issues appears.~~
Apparently the SPI bus shares a pin with a LED output which initialization function overrides the SPI output in my case, the additional periph_init() disables the GPIO mode of the pin again so that SPI comm is possible.

PeterKietzmann · 2017-01-26T09:33:26Z

That function is already called here. Tried your last step anyway but w/o success

haukepetersen · 2017-01-26T11:31:03Z

by the way, the handling of the init functions is quite strange:

the 'netdev'-init() calls mrf_setup(), which calls mrf_init()
the driver offers a set of public, netdev independent functions, but it can not be used stand-alone, as important init functionality is tied to the netdev implementation

Seems like this has been copied in parts from the very bad example of the at86rf driver?!

haukepetersen · 2017-01-26T11:34:23Z

Also the SPI bus is used quite inefficient -> the bus is 'acquired' for every single transfer. Ideally, this should be done only once for each block of transfers, e.g. 'acquire' bus access at the beginning of the initialization or a packet transfer and release it only after the initialization of the packet transfer is complete...

PeterKietzmann · 2017-01-26T11:35:48Z

Initial work has been done by students and yes, the atmel driver acted as a start point for that. @bergzand was so nice to take this PR over. However, we can clean up that driver afterwards (if we want to) but in case we can get this running with the SPI rework in the next 1-2 hours I'd vote for taking this PR in.

haukepetersen · 2017-01-26T11:38:14Z

Just wanted to state my concerns, I don't see them as blocker for this PR...

Regarding the 'not-working' state, I can't see anything in the code. But I don't have my device here so that I can help debugging. Have you tried running it with different boards?

bergzand · 2017-01-26T12:07:24Z

@haukepetersen thank you for your recommendations. I would prefer solving the issues after this PR, unless you consider them blocking for this PR. I would not be suprised if there are some bad practices copied from the at86rf2xx by me or @Nenaco. If you could create issues for them and assign them to me, I can solve them as soon as i've got time for them.

haukepetersen · 2017-01-26T12:36:19Z

@bergzand: I don't see them as blocker and they might as well be addressed in follow-up PR(s).

PeterKietzmann · 2017-01-26T13:52:48Z

Allright. After I figured out that nucelo-l1 boards are buggy, I can't flash my stm32f0discovery any more, this implementation does not work with stm32f4 and stm32f3 discovery I tried the the samr21-xpro, pulling out the dependency to the gnrc_netdev_defualt device. And tada, here it just works fine. So the problem seems not directly related to this driver but to SPI-connected devices with discovery boards. @haukepetersen would you still agree merging this PR?

PeterKietzmann · 2017-01-26T14:28:23Z

Ok I will merge this now. Already added it to the list in #6437. Please help making this driver work with the STM32Fx boards again. Feature freeze will be announces soon. Fixes should be backported to the release branches then. Don't forget!

bergzand · 2017-01-26T14:48:55Z

@PeterKietzmann Is it possible for you to check whether this is an issue with the mrf24j40 module or that the SPI module with the STM32Fx boards is broken somehow? I currently run the code from this PR on both a nucleo-f401 (stm32f401RE) and a maple-mini (stm32f103) so I would really like to know if I can reproduce your error. Could this be Nucleo-L1 specific? Furthermore, maybe the problem on the stm32f4 is caused by #6460.

haukepetersen · 2017-01-26T16:58:32Z

Actually, it might be, that there is something buggy in the STMs SPI driver (though I wonder how that happened after our extensive testing sessions for the new SPI driver): @kaspar030 was running into trouble today also, when testing the encx24j60 Ethernet driver with the nucleo-f334. Will investigate this tomorrow.

PeterKietzmann · 2017-01-27T16:06:51Z

@bergzand do you have a stm32f4discovery board available? I gave it a try again after #6492 was merged but still hangs somewhere during initialization. The radio works as expected with an Atmel samr21-xpro and stand-alone SPI and GPIO look fine on a stm32f4disco board. My configuration is:

#ifndef MRF24J40_PARAM_SPI
#define MRF24J40_PARAM_SPI          (SPI_DEV(0))
#endif
#ifndef MRF24J40_PARAM_SPI_CLK
#define MRF24J40_PARAM_SPI_CLK      (SPI_CLK_5MHZ)
#endif
#ifndef MRF24J40_PARAM_CS
#define MRF24J40_PARAM_CS           (GPIO_PIN(0, 4))
#endif
#ifndef MRF24J40_PARAM_INT
#define MRF24J40_PARAM_INT          (GPIO_PIN(4, 15))
#endif
#ifndef MRF24J40_PARAM_RESET
#define MRF24J40_PARAM_RESET        (GPIO_PIN(4, 13))
#endif

bergzand · 2017-01-28T16:42:04Z

@PeterKietzmann I've borrowed an stm32f4discovery from a friend and I couldn't get it to work either. It seems that it passes the communications check, but hangs when trying a rf state machine. Can you confirm that in your case it also hangs in a loop at https://github.com/RIOT-OS/RIOT/blob/master/drivers/mrf24j40/mrf24j40_getset.c#L476 ?

PeterKietzmann · 2017-01-30T16:01:57Z

Confirmed. @bergzand let's continue the discussion in #6519

bergzand changed the title ~~driver/mrf24j40: Add mrf24j40 radio support~~ drivers/mrf24j40: Add mrf24j40 radio support Dec 31, 2016

OlegHahm assigned PeterKietzmann Jan 3, 2017

OlegHahm added Area: drivers Area: Device drivers Area: network Area: Networking labels Jan 3, 2017

bergzand force-pushed the mrf24j40 branch from b02477d to 0d9f927 Compare January 8, 2017 14:59

bergzand force-pushed the mrf24j40 branch from 89d748f to b0ec452 Compare January 11, 2017 22:25

haukepetersen requested changes Jan 18, 2017

View reviewed changes

PeterKietzmann mentioned this pull request Jan 20, 2017

Mrf24j40 #5792

Closed

bergzand force-pushed the mrf24j40 branch 2 times, most recently from cdd7012 to dec8822 Compare January 20, 2017 10:41

bergzand force-pushed the mrf24j40 branch from dec8822 to f387cd7 Compare January 20, 2017 13:19

PeterKietzmann reviewed Jan 20, 2017

View reviewed changes

bergzand force-pushed the mrf24j40 branch 4 times, most recently from 97b991d to acb85e4 Compare January 21, 2017 10:14

bergzand force-pushed the mrf24j40 branch 2 times, most recently from 87cf8e4 to 2dc914c Compare January 25, 2017 21:10

PeterKietzmann added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Jan 25, 2017

bergzand force-pushed the mrf24j40 branch from 806b584 to 9530b4f Compare January 25, 2017 23:19

drivers/mrf24j40: initial mrf24j40 support

672da2c

bergzand force-pushed the mrf24j40 branch from 9530b4f to 672da2c Compare January 26, 2017 09:13

haukepetersen approved these changes Jan 26, 2017

View reviewed changes

PeterKietzmann merged commit 9fe7763 into RIOT-OS:master Jan 26, 2017

bergzand deleted the mrf24j40 branch September 29, 2017 09:09

drivers/mrf24j40: Add mrf24j40 radio support #6277

drivers/mrf24j40: Add mrf24j40 radio support #6277

Conversation

bergzand commented Dec 31, 2016

OlegHahm commented Jan 3, 2017

bergzand commented Jan 3, 2017

bergzand commented Jan 8, 2017

bergzand commented Jan 8, 2017

OlegHahm commented Jan 14, 2017

PeterKietzmann commented Jan 14, 2017

Nenaco commented Jan 15, 2017

haukepetersen commented Jan 18, 2017

PeterKietzmann commented Jan 18, 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

bergzand commented Jan 19, 2017

PeterKietzmann commented Jan 19, 2017

PeterKietzmann commented Jan 20, 2017

bergzand commented Jan 20, 2017 • edited Loading

bergzand commented Jan 20, 2017

PeterKietzmann 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

PeterKietzmann commented Jan 24, 2017

bergzand commented Jan 25, 2017

PeterKietzmann commented Jan 25, 2017

bergzand commented Jan 25, 2017

PeterKietzmann commented Jan 26, 2017

PeterKietzmann commented Jan 26, 2017

bergzand commented Jan 26, 2017 • edited Loading

PeterKietzmann commented Jan 26, 2017

haukepetersen commented Jan 26, 2017

haukepetersen commented Jan 26, 2017

PeterKietzmann commented Jan 26, 2017

haukepetersen commented Jan 26, 2017

bergzand commented Jan 26, 2017

haukepetersen commented Jan 26, 2017

PeterKietzmann commented Jan 26, 2017

PeterKietzmann commented Jan 26, 2017

bergzand commented Jan 26, 2017

haukepetersen commented Jan 26, 2017

PeterKietzmann commented Jan 27, 2017

bergzand commented Jan 28, 2017

PeterKietzmann commented Jan 30, 2017

bergzand commented Jan 20, 2017 •

edited

Loading

bergzand commented Jan 26, 2017 •

edited

Loading