cc1200: New device driver for TI sub-ghz radio

[uknoblic]

In our company's effort to develop a new platform (CC2538 + CC1200 + ENC28j60) i hereby commit the driver for the CC1200 radio. Even if it borrows some ideas e.g. from the cc1120 or cc2538 radio driver, it was completely re-worked, is highly configurable and also supports 802.15.4g (packet length > 127 bytes). It also uses the channel spacing defined in 802.15.4g for ETSI and FCC. It is currently running stable at 50kbps (ETSI) using nullrdc mac-driver. To my knowledge it will NOT WORK with the cc1120 or cc1101 drivers at it uses at least another channel spacing. Hopefully we will be able to achieve compatibility with old (and upcoming) sub-ghz radios one day...

I would again like to raise the question if we shouldn't consider a re-work of way we handle rf transmission within contiki. As we are currently blocking within "transmit()", we will hardly be able to use features like bigger packets or wake-up preambles!

I would be happy to get feedback!

[jonnteolsson]

Great addition! I know you have made tests with other data rates, it would be useful for people to understand how other RF settings can be exported from SmartRF Studio.

[alignan]

Great contribution!

There are some files without proper code style formatting (the cc1200.c has many), please run uncrustify and push -f.

[g-oikonomou]

Same as #1002, this feature is mega-welcome. I shall try it as soon as I get a chance!

I would again like to raise the question if we shouldn't consider a re-work of way we handle rf transmission within contiki. As we are currently blocking within "transmit()", we will hardly be able to use features like bigger packets or wake-up preambles!

This is something that has been spinning around in my head for a long while now. IMHO we should be allowed to pass a frame to the DMA controller if one exists and then let rip! But this is probably a discussion more suitable for another pull/issue.

[g-oikonomou]

This looks overall very very tidy. As Antonio said, it needs a little uncrustify love.

Can you briefly explain the situation about the .inc file? How do things work? Is it a direct dump from SRF Studio? I see that we are including one among a list of possible .inc files, but this pull only presents one of them. Is this because of where you have focused your testing efforts?

It should be easy to typedef registerSetting_t register_setting_t within the driver's main file in order to get rid of the camelCase naming. But before spending any time on that, I first want to understand how those inc files work.

Thanks for your efforts!

[g-oikonomou]

Can you briefly explain the situation about the .inc file? How do things work? Is it a direct dump from SRF Studio?

OK now I see how it works.

As a bare minumum, I would change .inc to .h. I also want to clarify copyright etc for stuff exported by SRF Studio. Ideally we can have confirmation that it's OK to redistribute under the 3-clause BSD. Have you spoken to TI about this?

Beyond that, it depends how clever we want to get. For example, we can specify a contiki template for SRF Studio that will export a global symbol including register settings as well as other stuff such as packet lifetimes and whatever else changes across modes. With this in place, we can then get rid of the mega #if block in the main driver file and replace it with something as simple as a configuration directive.

Totally untested, but here's the rough idea:

• In cc1200.h we specify

typedef struct cc1200_register_setting {
  uint16_t addr;
  uint8_t val;
} cc1200_register_setting_t;

typedef struct cc1200_mode {
  cc1200_register_setting_t *register_settings;
  rtimer_clock_t packet_lifetime;
  uint32_t whatever_else_we_need;
} cc1200_mode_t;

• In our settings file exported by SRF Studio, we do something like:

#include "cc1200.h"
/*---------------------------------------------------------------------------*/
static const cc1200_register_setting_t register_setting[] = {
  {CC1200_IOCFG2,            0x06},
  {CC1200_DEVIATION_M,       0xD1},
  {CC1200_XOSC1,             0x03},
};
/*---------------------------------------------------------------------------*/
/* Global linkage: symbol name must be different in each exported file */
const cc1200_mode_t cc1200_mode_etsi_gfsk_50kpbs = {
  register_setting,
  (RTIMER_SECOND / 20),
  0x1234ABCD
};

• In our configuration file, we do #define CC1200_CONF_MODE cc1200_mode_etsi_gfsk_50kpbs.

[alignan]

That's actually how we did in TinyOS and my idea of having for the cc1120, as we had a project in which we had to support support for different configurations (FCC, CEPT, ARIB) and then variant-specifics (Thailand, Australia, etc).

[uknoblic]

I agree with the idea to use a single file to put the mode specific stuff into, but maybe it's not a good idea to use a SmartRF template for this purpose. As a compromise i would propose to put all the relevant stuff into on file for each mode, and let the user copy / paste the SmartRF settings using a default template.

[uknoblic]

By the way: in the meantime i fixed the indentations problems and a bug in the sniffer functionality.

[laurentderu]

With a good template, SmartRF can create files almost compliant with Contiki coding style. And, as I learned the hard way, some of the reserved register of the CC1200 must be set to certains values that depends on the configuration (and that smartRF knows) otherwise the radio does not work properly.

What could be done is include the configuration file using a macro so you don't have to use a huge #if switch case, for example :

  #ifdef RADIO_PROFILE_H
  #include RADIO_PROFILE_H
  #else
  #error RADIO_PROFILE not defined
  #endif

Also it would be nice to print this profile name in the init() function, it's much easier to detect that you are trying to communicate to an EU profile with an US profile :)

Small remark about the comments, there are quite a bunch of "/* func(), uk 21.05.2015 */" at the end of functions, I don't think they are really useful.

[uknoblic]

With a good template, SmartRF can create files almost compliant with Contiki coding style. And, as I learned the hard way, some of the reserved register of the CC1200 must be set to certains values that depends on the configuration (and that smartRF knows) otherwise the radio does not work properly.

Ok, but wheres the problem when i use the registers exported from smartRF? I shouldn't miss one if smartRF works reliable? I am aware of the fact that one should not try to specify the registers from the user's guide alone...

Small remark about the comments, there are quite a bunch of "/* func(), uk 21.05.2015 */" at the end of functions, I don't think they are really useful.

I will remove them in one of my next commits. It's just a reminder for me that i have reviewed the corresponding function.

I agree with the idea to use a single file to put the mode specific stuff into, but maybe it's not a good idea to use a SmartRF template for this purpose. As a compromise i would propose to put all the relevant stuff into on file for each mode, and let the user copy / paste the SmartRF settings using a default template.

Comming back to this proposal: From my experience there is sometimes the need to re-export the settings from smartRF. If we put all the stuff (like rf timeouts, min-channel, max-channel, max txpower...) in a smartrf-template, then you have to re-edit all this (basic) stuff each time you change a small parameter which to my felling is error-prone. Comments are welcome!

[uknoblic]

I have added two files taking into account your proposals regarding the mode configuration. I didn't test / compile it (i am on my windows pc), is just for review...

[uknoblic]

I have added two files taking into account your proposals regarding the mode configuration. I didn't test / compile it (i am on my windows pc), is just for review...

Revised my last commit. Comments are welcome.

[g-oikonomou]

Comming back to this proposal: From my experience there is sometimes the need to re-export the settings from smartRF. If we put all the stuff (like rf timeouts, min-channel, max-channel, max txpower...) in a smartrf-template, then you have to re-edit all this (basic) stuff each time you change a small parameter which to my felling is error-prone. Comments are welcome!

I am not sure I understand what you are trying to say here, Ulf. Do you mean the stuff not exported by SRF Studio like channel min, max etc?

My original thinking about a SRF Studio template was under the provision that the user would only have to change 1, max 2 things after export. If we cannot achieve that, then we should forget about it and proceed as you suggest with register settings copy-paste.

Some additional comments:

• Sniffer stuff is problematic I'm afraid, and this will start to surface when people start tryng to build for different platforms. For example, platforms that don't have a usb-serial.h or platforms that use uart1_write(b) instead of uart_write(1, b). I don't have a solution for that I'm afraid, otherwise I'd have put it forward for border routers and similar examples too. This file being in core though, warrants a platform-independent approach.

Some minor stuff inline

[g-oikonomou]

No need for this conditional here. You can simply just include it

[laurentderu]

I'm going to test it on exp5438 in the following days, I got it compiling for it already. There are a few compiler warnings that you should remove :

cc1200_driver.prepare
cc1200_driver.transmit
cc1200_driver.send
cc1200_driver.read

have incompatible signatures wrt struct radio_driver.

line 2453: the second argument for burst_read has incompatible pointer type.

[uknoblic]

Sorry, i was to fast pushing my new versions. I will squash all commits considering your remarks.

Comming back to this proposal: From my experience there is sometimes the need to re-export the settings from smartRF. If we put all the stuff (like rf timeouts, min-channel, max-channel, max txpower...) in a smartrf-template, then you have to re-edit all this (basic) stuff each time you change a small parameter which to my felling is error-prone. Comments are welcome!

I am not sure I understand what you are trying to say here, Ulf. Do you mean the stuff not exported by SRF Studio like channel min, max etc?

Yes, this is what i wanted to say. Have a look at the two new files (cc1200-rf-cfg.h + cc1200-802154g-863-870-fsk-50kbps.c)

[uknoblic]

Sniffer stuff is problematic I'm afraid, and this will start to surface when people start tryng to build for different platforms. For example, platforms that don't have a usb-serial.h or platforms that use uart1_write(b) instead of uart_write(1, b). I don't have a solution for that I'm afraid, otherwise I'd have put it forward for border routers and similar examples too. This file being in core though, warrants a platform-independent approach.

So just move the sniffer output from cc1200.c and place it in stub-rdc.c? This would be easy...

[g-oikonomou]

So just move the sniffer output from cc1200.c and place it in stub-rdc.c? This would be easy...

There have been some discussions about knocking together a generic sniffer and the first step (out of many) would be exactly what you suggest.

In the context of this pull request, I'd move the sniffer stuff out of the driver to start with. I suspect there will be an example of how to use this driver at some point in the future with the Weptech platform. An accompanying device-specific sniffer example would be a nice addition to that bundle.

I'll take a closer look at your updated proposal for the settings, but at first glance it looks much better this way.

Thanks!

[alignan]

Tested with CPOL=1 and CPHA=0 and worked, checked with a logic analyser, otherwise failed.

[alignan]

I have tested the PR over some time now, I have some enhancements as well. I'm ready to merge as it is (I have only made a small comment).

[uknoblic]

This define doesn't work at this place. Remove it from here.

[alignan]

👍