Skip to content

sx127x: ignore empty iolist element#11215

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
jia200x:pr/sx127x_fix
Apr 1, 2019
Merged

sx127x: ignore empty iolist element#11215
miri64 merged 1 commit intoRIOT-OS:masterfrom
jia200x:pr/sx127x_fix

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Mar 20, 2019

Contribution description

fixes #11163 for the sx127x radio driver

Testing procedure

Apply the following patch:

diff --git a/tests/driver_sx127x/main.c b/tests/driver_sx127x/main.c
index a009e7a2f..92fa0d18f 100644
--- a/tests/driver_sx127x/main.c
+++ b/tests/driver_sx127x/main.c
@@ -236,8 +236,8 @@ int send_cmd(int argc, char **argv)
            argv[1], (unsigned)strlen(argv[1]) + 1);
 
     iolist_t iolist = {
-        .iol_base = argv[1],
-        .iol_len = (strlen(argv[1]) + 1)
+        .iol_base = NULL,
+        .iol_len = 0
     };
 
     netdev_t *netdev = (netdev_t*) &sx127x;

Compile and flash wit b-l072z-lrwan1 or any compatible LoRaWAN board.
Without this PR, it will crash due to an SPI assertion

Issues/PRs references

#11163

@jia200x jia200x added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: LoRa Area: LoRa radio support labels Mar 20, 2019
@jia200x jia200x requested review from aabadie and miri64 March 20, 2019 09:53
@jia200x jia200x added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Mar 20, 2019
@miri64 miri64 mentioned this pull request Mar 20, 2019
Closed
24 tasks
miri64
miri64 reviewed Mar 26, 2019
View reviewed changes
miri64
miri64 reviewed Mar 27, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Change looks reasonable, I don't have the hardware however to test.

@miri64 miri64 added Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines and removed Impact: major The PR changes a significant part of the code base. It should be reviewed carefully Impact: minor The PR is small in size and might only require a quick look of a knowledgeable reviewer labels Mar 27, 2019
@miri64
Copy link
Copy Markdown
Member

miri64 commented Mar 27, 2019

So soft ACK from my side. @aabadie can you test.

smlng
smlng suggested changes Mar 29, 2019
View reviewed changes
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Apr 1, 2019

@smlng the _get_state call doesn't call any SPI routines (just returns the value of an internal variable). Both guards have very low chances to be called (since most of the time is running LoRaWAN which follows some slots) and in case of raw sx127x, I'm pretty sure there will be more calls to the _get_state than the size==0 guard. So I would probably leave it as it is ;)

@miri64 miri64 added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Apr 1, 2019
@miri64 miri64 merged commit 299a190 into RIOT-OS:master Apr 1, 2019
@danpetry danpetry added this to the Release 2019.04 milestone Apr 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@miri64 miri64 miri64 left review comments

@aabadie aabadie Awaiting requested review from aabadie aabadie is a code owner

+1 more reviewer

@smlng smlng smlng approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

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 Reviewed: 1-fundamentals The fundamentals of the PR were reviewed according to the maintainer guidelines Reviewed: 2-code-design The code design of the PR was reviewed according to the maintainer guidelines Reviewed: 4-code-style The adherence to coding conventions by the PR were reviewed according to the maintainer guidelines Reviewed: 5-documentation The documentation details of the PR were reviewed according to the maintainer guidelines Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors)

Projects

None yet

Milestone

Release 2019.04

Development

Successfully merging this pull request may close these issues.

netdev: iolist not checked for empty elements on send

4 participants

@jia200x @miri64 @smlng @danpetry