Skip to content

drivers/sx127x: Fix misc variable types and lengths#7851

Merged
kYc0o merged 4 commits intoRIOT-OS:masterfrom
kYc0o:fix_sx127x_misc
Oct 26, 2017
Merged

drivers/sx127x: Fix misc variable types and lengths#7851
kYc0o merged 4 commits intoRIOT-OS:masterfrom
kYc0o:fix_sx127x_misc

Conversation

@kYc0o
Copy link
Copy Markdown
Contributor

@kYc0o kYc0o commented Oct 26, 2017

While compiling tests/driver_sx127x with clang in OS X it discovers several minor issues.

This PR fixes them.

@kYc0o kYc0o added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: drivers Area: Device drivers Area: LoRa Area: LoRa radio support Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 26, 2017
@kYc0o kYc0o requested a review from aabadie October 26, 2017 00:15
Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

Almost good, just one remark on the power type change.

typedef struct {
uint16_t preamble_len; /**< Length of preamble header */
uint8_t power; /**< Signal power */
int8_t power; /**< Signal power */
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This change doesn't seem to be related to compiler issues but it's a valid one regarding LoRa: the power in dBm can indeed be negative.

Note that you also need to update the netdev _set function according to this change.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

clang rise an error of assigning negative values to this variable. Besides the fact that yes, conceptually this should be a uint8_t.

The netdev change was already done, that's why Murdock agrees.

Copy link
Copy Markdown
Contributor

@aabadie aabadie Oct 26, 2017

Choose a reason for hiding this comment

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

I think you misunderstood my comment. Using an int8_t is correct since power can be negative. But the netdev _set function should be adapted as well and this is not done by this PR. See this line
The line just under is also erroneous as it returns sizeof(uint16_t) and it should be sizeof(int8_t according to this PR.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Ok got it. Fixed.

Copy link
Copy Markdown
Contributor

@aabadie aabadie left a comment

Choose a reason for hiding this comment

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

ACK, please squash

@kYc0o
Copy link
Copy Markdown
Contributor Author

kYc0o commented Oct 26, 2017

Go!

@kYc0o kYc0o merged commit 1baece5 into RIOT-OS:master Oct 26, 2017
@kYc0o kYc0o mentioned this pull request Oct 26, 2017
16 tasks
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: LoRa Area: LoRa radio support CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants