Skip to content

drivers/cc110x: Allow packet sizes up to 255 bytes#2237

Closed
fnack wants to merge 1 commit intoRIOT-OS:masterfrom
fnack:cc110x_pl
Closed

drivers/cc110x: Allow packet sizes up to 255 bytes#2237
fnack wants to merge 1 commit intoRIOT-OS:masterfrom
fnack:cc110x_pl

Conversation

@fnack
Copy link
Copy Markdown
Member

@fnack fnack commented Dec 30, 2014

This PR extends the current cc110x driver implementation to allow packet sizes up to 255 bytes (maximum for Variable Packet Length Mode) and adapts the netdev receive mechanism to handle packet reception not in the interrupt but in the event handling thread. (UPDATE: Old netdev is deprecated, therefore no longer handled in this PR).

Both, the RX and the TX FIFO of the cc110x can only store 64 bytes. Therefore, for packet sizes > 64 bytes consecutively reading/pushing bytes is necessary.

For transmission of packets > 64 bytes the TX FIFO is filled with the first 64 bytes and afterwards the remaining bytes are consecutively pushed to the TX FIFO when space is available until all the bytes are transmitted (necessary because a TXFIFO underflow would result in a corrupted packet).

For reception of a packet, two interrupts are used. The first interrupt is triggered, when the RX FIFO is filled with more than 4 bytes (RX FIFO threshold, packets with less bytes are not valid). Subsequently, the available bytes in the RXFIFO are consecutively read until the whole packet is received. This is necessary to avoid a RX FIFO overflow. The second interrupt is triggered when the cc110x signals that the whole packet was received. This results in reading the remaining two status bytes in the RXFIFO. Afterwards the packet either gets dropped (CRC wrong) or gets processed further.

The PR is depending on #2163 (introduction of event handler pid member variable in netdev_t).(UPDATE: Old netdev is deprecated, this PR no longer relies on netdev changes).

@fnack fnack added State: waiting for other PR State: The PR requires another PR to be merged first Area: drivers Area: Device drivers Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation labels Dec 30, 2014
@fnack
Copy link
Copy Markdown
Member Author

fnack commented Dec 30, 2014

Please note that for packets <= 64 bytes the driver implementation is still interoperable with devices using the cc110x_legacy driver.

@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 31, 2014

What is the use-case? How do you handle proper reassembly?

Personally, I feel like this is not something the driver should do. A packet is a unit that the device gets over the air and this should be presented as such to higher layers. Everything else should be left to the higher layers (e.g. 6LoWPAN and/or IPv6)

@fnack
Copy link
Copy Markdown
Member Author

fnack commented Dec 31, 2014

I don't really understand your comment. Maybe my description was confusing.

What is the use-case:
Payload handed to the cc110x for data transmission can now be up to 252 bytes (255 - 3 bytes that are needed for cc110x specific headers). Previously, the maximum payload size was 58 bytes. If the payload exceeded this size no data was send.

How do you handle proper reassembly:
See my PR description?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Dec 31, 2014

Payload handed to the cc110x for data transmission can now be up to 252 bytes (255 - 3 bytes that are needed for cc110x specific headers). Previously, the maximum payload size was 58 bytes. If the payload exceeded this size no data was send.

This is not a use-case IMHO, but a nice-to-have feature. If someone wants to send packets as large as this she should use an upper layer that can deal with this (6LoWPAN e.g.). In particular, 6LoWPAN also actually deals with out-of-order arrivals (which your implementation does not, if I see that correctly) and overlapping fragments, so I'm not convinced, that we need this.

@fnack
Copy link
Copy Markdown
Member Author

fnack commented Jan 1, 2015

I don't understand your concerns with this pull request. Yes, my explanation might only describe a feature but I think we are talking at cross purposes. An application simply exchanging a full set of Sensor data between two nodes is an easy example. This data could easily grow larger than 58 bytes. Why should you force the developer to use upper layer mechanisms so that data fragmentation is handled if for such an easy application this would only introduce computational and data overhead (for multiple packets).

It especially confuses me that your don't like the idea of extending a current device implementation to use the (full) capabilities of this device. If you don't want to use this feature, simply don't use it.

The CC110X device does no processing of an incoming packet but simply hands it to its upper layer. Of course there is no handling of out of order arrivals. How should the CC110X know which order is the correct one? That is always in the scope of upper layers.

@fnack
Copy link
Copy Markdown
Member Author

fnack commented Jan 1, 2015

I hope that @OlegHahm jumps to the rescue soon and gives his opinion because I don't think we'll come to a conclusion with our discussion ;)

@LudwigKnuepfer
Copy link
Copy Markdown
Member

@authmillenon Maybe you missed that this PR is actually implementing something the device supports (i.e. not some hack)?

@miri64
Copy link
Copy Markdown
Member

miri64 commented Jan 5, 2015

After reading a little bit into the related datasheets I see that you're right @LudwigOrtmann. Is there a way of getting this to run on MSBA2?

@fnack
Copy link
Copy Markdown
Member Author

fnack commented Jan 5, 2015

First possibility - Adapt the legacy driver:
I currently don't want to put effort in adapting the CC110X_legacy driver. The drivers are structured different and for the legacy driver the unique code for every board that uses it would have to be adapted. And it's a legacy driver for a reason, so it should be removed anyway after old boards are migrated.

Second possibility - Use the new driver on MSBA2:
To get it to run on MSBA2, the periph/gpio and spi low-level interfaces would have to be implemented and the MSBA2's config would need to be changed to use the new driver.

Second possibility should be the goal for the future!

@LudwigKnuepfer
Copy link
Copy Markdown
Member

The first possibility should not be considered in my opinion ;)

@OlegHahm OlegHahm added the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Jan 6, 2015
@OlegHahm
Copy link
Copy Markdown
Member

Is this now dependent on #2398?

@fnack
Copy link
Copy Markdown
Member Author

fnack commented Feb 22, 2015

Yes, the CC110x driver has to be adapted to the new ng_netdev interface. I'm waiting for Thomas' PR for the adapted at86 radio driver to get an impression on what needs to be done.

But the basic rx and tx functionality from this PR won't change with the adaption.

@PeterKietzmann PeterKietzmann removed the Community: Hack'n'ACK candidate This PR is a candidate for review and discussion during one of RIOT's monthly Hack'n'ACK parties label Mar 3, 2015
@fnack fnack removed Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. State: waiting for other PR State: The PR requires another PR to be merged first labels Mar 6, 2015
@fnack
Copy link
Copy Markdown
Member Author

fnack commented Mar 6, 2015

Rebased.

@OlegHahm: I changed this PR so that it no longer touches any netdev stuff and only changes the rx and tx routines of the driver to allow packets of up to 255 bytes. As we are still missing a working implementation of the ng_netdev interface for any device, I think this is the better approach. Therefore, the PR is ready for review!

@OlegHahm OlegHahm force-pushed the master branch 2 times, most recently from 59b8450 to 9f184dd Compare March 31, 2015 12:53
@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Apr 9, 2015

Sorry for the long delay. The problem that I have is that reviewing device specific code without having the device is a bit difficult. Does anyone can get his hands on at least two MSB-IoTs?

@OlegHahm
Copy link
Copy Markdown
Member

OlegHahm commented Apr 9, 2015

@haukepetersen, maybe you?

@OlegHahm OlegHahm assigned haukepetersen and unassigned OlegHahm Apr 13, 2015
@haukepetersen
Copy link
Copy Markdown
Contributor

I will have a look, but I will not get to it this week...

@OlegHahm OlegHahm modified the milestone: Release 2015.06 Apr 29, 2015
@OlegHahm
Copy link
Copy Markdown
Member

@kaspar030, @haukepetersen, can we close this?

@kaspar030
Copy link
Copy Markdown
Contributor

I guess so, the driver has been obsoleted, and I have a netdev2-version ready that also supports large packets.

@kaspar030 kaspar030 closed this Aug 12, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: drivers Area: Device drivers Type: enhancement The issue suggests enhanceable parts / The PR enhances parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants