Skip to content

Conversation

miri64
Copy link
Member

@miri64 miri64 commented Jan 18, 2016

This proposes an API change to pass up packet status information like RSSI or LQI to upper-layers. The CC11xx driver circumvents this problem this by (seemingly?) bypassing the netdev2 API altogether for GNRC and for my preliminary netdev2 port of IEEE 802.15.4 (#4645) I just appended this information to the packet, as most device drivers do anyway. With the last decision I'm not happy however and this PR tries to improve the overall situation.

@miri64 miri64 added Area: network Area: Networking Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care. Area: drivers Area: Device drivers labels Jan 18, 2016
@miri64 miri64 added this to the Release 2016.03 milestone Jan 18, 2016
@haukepetersen
Copy link
Contributor

NACK in it's current state -> IMHO netdev2 is link layer agnostic, and I don't like a solution, in which ethernet devices have to hand over radio specific information (at least conceptual, though in practice you would just pass NULL I guess).

Now lets see if there is a better solution: if we would keep strictly to the plain concept of netdev(2), one would acquire the RSSI and LQI values on demand using the get() function. Now this would mean to calls to get on each received packet - screw that, too inefficient...

Adding an additional function like get_rx_info() can I think also be ruled out, as we don't want to bloat the interface...

So how about generalizing this PR just slightly:

int (*recv)(netdev2_t *dev, char* buf, int len, void *info);

and make the type of info depend on the type of device (which you can find out with get(NETOPT_DEVICE_TYPE)). So all 15.4 devices could then pass an instance of

typedef struct {
    uint8_t rssi;       /**< RSSI of a received packet */
    uint8_t lqi;        /**< LQI of a received packet */
} netdev2_radio_info_t;

when receiving a packet?

This paradigm could be even extended to sending packets. So by changing the send function to

int (*send)(netdev2_t *dev, const struct iovec *vector, int count, void *info);

we could give link layer specific information back, as for example the number of retries (if available...). But if this makes sense we should discuss separately...

@miri64
Copy link
Member Author

miri64 commented Jan 23, 2016

Now lets see if there is a better solution: if we would keep strictly to the plain concept of netdev(2), one would acquire the RSSI and LQI values on demand using the get() function. Now this would mean to calls to get on each received packet - screw that, too inefficient...

I'm with you on that. I already need to call a lot of get, with netdev2 (to get the source address and its length e.g.), I like to keep this to a minimum.

So how about generalizing this PR just slightly:

int (*recv)(netdev2_t *dev, char* buf, int len, void *info);

and make the type of info depend on the type of device (which you can find out with get(NETOPT_DEVICE_TYPE)). So all 15.4 devices could then pass an instance of

typedef struct {
    uint8_t rssi;       /**< RSSI of a received packet */
    uint8_t lqi;        /**< LQI of a received packet */
} netdev2_radio_info_t;

when receiving a packet?

I had it like this first, but than decided it to do it as it is currently in the PR… don't know why I changed it... Will do.

This paradigm could be even extended to sending packets. So by changing the send function to

int (*send)(netdev2_t *dev, const struct iovec *vector, int count, void *info);

we could give link layer specific information back, as for example the number of retries (if available...). But if this makes sense we should discuss separately...

Symmetry is always better :-). I think I will provide this API change, but not the type needed for it for now.

@miri64 miri64 force-pushed the netdev2/api/packet-info branch from 926ca1f to 693bc0f Compare January 23, 2016 12:30
@miri64
Copy link
Member Author

miri64 commented Jan 23, 2016

Done.

@haukepetersen
Copy link
Contributor

looks ok to me. One thing we should spend a thought about is the naming scheme for the info types (as netdev2_radio_recv_info_t). I think it makes sense to couple them somehow to the netdev types, so that you can at least guess the correct info type before you have to read any documentation. What I mean is something like netdev2_802154_rx_info_t, netdev2_cc110x_rx_info_t, etc, while the two might as well be type deved to the same structure... Makes sense?

@haukepetersen haukepetersen reopened this Jan 25, 2016
@miri64 miri64 force-pushed the netdev2/api/packet-info branch from b99b0b8 to 35afa05 Compare January 25, 2016 11:07
@miri64
Copy link
Member Author

miri64 commented Jan 28, 2016

@haukepetersen ping?

@OlegHahm
Copy link
Member

I guess @haukepetersen meant something like:

struct netdev2_radio_rx_info {
    uint8_t rssi;       /**< RSSI of a received packet */
    uint8_t lqi;        /**< LQI of a received packet */
};

typedef struct netdev2_radio_rx_info netdev2_ieee802154_rx_info_t;
typedef struct netdev2_radio_rx_info netdev2_cc110x_rx_info_t;

@miri64
Copy link
Member Author

miri64 commented Jan 29, 2016

Done and adapted existing drivers. Will introduce netdev2_ieee802154_rx_info_t in netdev2_ieee802154.h as soon as #4645 gets merged (or if this one gets merged first I will adapt it here).

@OlegHahm
Copy link
Member

What's the intention of the info parameter for the send function? Any use case in mind?

@miri64
Copy link
Member Author

miri64 commented Jan 30, 2016

From #4648 (comment):

This paradigm could be even extended to sending packets. So by changing the send function to

int (*send)(netdev2_t *dev, const struct iovec *vector, int count, void *info);

we could give link layer specific information back, as for example the number of retries (if available...). But if this makes sense we should discuss separately...

@OlegHahm
Copy link
Member

What happened to the good old: "implement that when there is a (concrete) need for it"? ;-)

@miri64
Copy link
Member Author

miri64 commented Jan 31, 2016

I did not implement anything, I just provided an API to do it ;-)

@haukepetersen
Copy link
Contributor

What happened to the good old: "implement that when there is a (concrete) need for it"? ;-)

Now looking at this PR again I think @OlegHahm has a point -> how about we keep the send function untouched for now and add it when needed?

Also I would like @kaspar030 to take a look at this change and get his approval.

@miri64
Copy link
Member Author

miri64 commented Feb 9, 2016

how about we keep the send function untouched for now and add it when needed?

Done

@OlegHahm OlegHahm added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 22, 2016
@OlegHahm
Copy link
Member

ACK

@miri64 miri64 force-pushed the netdev2/api/packet-info branch from 07f76cf to b7735c5 Compare February 22, 2016 10:32
@miri64
Copy link
Member Author

miri64 commented Feb 22, 2016

Rebased and squashed.

*/
struct netdev2_radio_rx_info {
uint8_t rssi; /**< RSSI of a received packet */
uint8_t lqi; /**< LQI of a received packet */
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@OlegHahm how about including the number of retries here? But this might be best targeted in another PR?!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Number of retries in the RX info?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ups, mixed up contexts. Never mind...

@haukepetersen
Copy link
Contributor

ACK also from my side.

@miri64 miri64 force-pushed the netdev2/api/packet-info branch from b7735c5 to 29bd517 Compare February 22, 2016 11:35
@miri64
Copy link
Member Author

miri64 commented Feb 22, 2016

Fixed some errors and squashed immediately

@miri64 miri64 force-pushed the netdev2/api/packet-info branch from 29bd517 to bd8d2d3 Compare February 22, 2016 14:15
@miri64
Copy link
Member Author

miri64 commented Feb 22, 2016

Can someone look over the changes and say if they are still okay?

@kaspar030
Copy link
Contributor

Looks ok to me.

The doc says the info's datatype is "depending on the device type". That means NETDEV2_DEVICE_TYPE? Or how can I find out which type exactly?

@miri64
Copy link
Member Author

miri64 commented Feb 22, 2016

By name ;)

miri64 added a commit that referenced this pull request Feb 22, 2016
netdev2: provide capability to pass up packet status information
@miri64 miri64 merged commit 0018bd9 into RIOT-OS:master Feb 22, 2016
@miri64 miri64 deleted the netdev2/api/packet-info branch February 22, 2016 20:20
@OlegHahm OlegHahm mentioned this pull request Mar 9, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Discussion: RFC The issue/PR is used as a discussion starting point about the item of the issue/PR Process: API change Integration Process: PR contains or issue proposes an API change. Should be handled with care.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants