Inconsistent Buffer Size Specification for Send

[mattbrown015]

Description

There's something inconsistent about the type of the size parameter in send and the default configuration of MBED_CONF_SX1276_LORA_DRIVER_BUFFER_SIZE.

The default value of MBED_CONF_SX1276_LORA_DRIVER_BUFFER_SIZE is 256 but size is a uint8_t and therefore has a maximum value of 255.

This is a bit annoying because either:

uint8_t send_buffer[MAX_DATA_BUFFER_SIZE_SX1276];
...
radio.send(&send_buffer[0], sizeof(send_buffer));

or

radio.send(&send_buffer[0], MAX_DATA_BUFFER_SIZE_SX1276);

Generates a compiler warning:

[Warning] main.cpp@48,64: large integer implicitly truncated to unsigned type [-Woverflow]

And casts doubt about what size the buffer can/should be and what's happened to the size parameter. My guess is that the default value of 'buffer size' should be 255.

Although, actually I'll probably only send a small packet to start with.

I think the underlying problem is probably the lack of status return codes in the driver interface. A better solution would be to make size a size_t, check it in send and return 'invalid parameter' if necessary.

I've got a similar concern with set_tx_config which does while (1) if it doesn't like one of the parameters.

SHA 5532d9d

Issue request type

[ ] Question [ ] Enhancement [X] Bug

[ciarmcom]

ARM Internal Ref: IOTCELL-1164

[mattbrown015]

While thinking about something else I happened to open the SX1276 datasheet and came across the description of the maximum payload length register:

RegMaxPayloadLength (0x23) 7-0 PayloadMaxLength(7:0) rw 0xff Maximum payload length; if header payload length exceeds value a header CRC error is generated. Allows filtering of packet with a bad size.

I think this means the maximum buffer size should be 255.

[hasnainvirk]

That's right. Will send a patch shortly.

[hasnainvirk]

Regarding returning error code from set_tx_config(), I would request to wait a while now. This driver code is largely inherited from Semtech implementation. We will attempt to improve the driver code base as soon as we have bandwidth available. For the time being I would suggest to get rid of while(1) and add an MBED_ASSERT. It's a bit presumptuous, but its kind of safe to assume that the only user of this driver is our stack and we do have appropriate checks for the bandwidth checks over there.

[hasnainvirk]

@mattbrown015 Please check the PR opened. https://github.com/ARMmbed/mbed-semtech-lora-rf-drivers/pull/26

[mattbrown015]

I agree MBED_ASSERT is a better choice than while(1) and understand that reworking the API is not the most important job at the moment; I was just thinking ahead.

Thanks, Matt