Skip to content

sx127x: add several NETOPT for GNRC LoRaWAN#11736

Merged
aabadie merged 4 commits intoRIOT-OS:masterfrom
jia200x:pr/sx127x_netops
Jun 28, 2019
Merged

sx127x: add several NETOPT for GNRC LoRaWAN#11736
aabadie merged 4 commits intoRIOT-OS:masterfrom
jia200x:pr/sx127x_netops

Conversation

@jia200x
Copy link
Copy Markdown
Member

@jia200x jia200x commented Jun 24, 2019 •
edited
Loading

Contribution description

This PR add the following NETOPT to the SX127x driver:

  • NETOPT_RANDOM: Get a random number from the LoRa device
  • NETOPT_SYNCWORD: Get/set the LoRa PHY Syncword.
  • NETOPT_RX_SYMBOL_TIMEOUT: Set the number of symbols the LoRa transceiver should wait before triggering an RX_TIMEOUT event, when set into RX_SINGLE mode.

These commits were cherry-picked from #11022

Testing procedure

Compile and flash the drivers_sx127x test:

BOARD=b-l072z-lrwan1 make -C tests/driver_sx127x all flash term

Then:

  • Verify the random command give random numbers
  • Use the syncword command to get and set the syncword.
    • Set a syncword (in decimal) with "syncword set "
    • Get the syncword (in hex) with "syncword get". Check both numbers are equivalent
    • Also, check the same hex number in 0x39 register with register get 0x39
  • Use the rx_timeout command to set the RX symbol timeout
    • rx_timeout set <dec>
    • Read the content of the registers 0x1E and 0x1F (rx_timeout is 10 bits long and scattered in these 2 registers). The 2 MSB of the rx_timeout are the 2 LSB of the 0x1E register and the 8 LSB are in the 0x1F reg. Check the hex matches the introduced rx_timeout value.
      E.g
1561389398.686839;st-lrwan1-11;New rx_timeout set
st-lrwan1-11;register get 0x1E
1561389453.166605;st-lrwan1-11;> register get 0x1E
1561389453.168076;st-lrwan1-11;[regs] 0x1E = 0xC4
st-lrwan1-11;register get 0x1F
1561389457.242929;st-lrwan1-11;> register get 0x1F
1561389457.245584;st-lrwan1-11;[regs] 0x1F = 0x32

note ((0xC4 & 0x3) << 8) | 0x32 == 0x32 => 50 decimal

Issues/PRs references

@jia200x jia200x requested review from aabadie and smlng June 24, 2019 15:30
@jia200x jia200x mentioned this pull request Jun 24, 2019
Merged
aabadie
aabadie reviewed Jun 24, 2019
View reviewed changes
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.

You can try the following patch

Why not adding a commit with these changes in the test application ?

@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jun 25, 2019

Why not adding a commit with these changes in the test application ?

Because I thought it's too hacky for its own commit. I think it would be possible to write unit tests for these get/set options (and check values with sx127x_reg_read). I can do it at some point

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 25, 2019

Because I thought it's too hacky for its own commit

I don't get your point.
Replacing sx127x_random by the corresponding netdev get function is straight forward. For the 2 other commands, you provided it in the patch, so should be ok to integrate it.
It'll also give a way to build (and manually test) the new netopts.

@jia200x jia200x force-pushed the pr/sx127x_netops branch 2 times, most recently from a549b46 to 19627a7 Compare June 25, 2019 14:46
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jun 25, 2019

@aabadie done!

@jia200x jia200x added 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 labels Jun 25, 2019
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jun 25, 2019

this is a dependency of #11022

smlng
smlng previously requested changes Jun 26, 2019
View reviewed changes
aabadie
aabadie reviewed Jun 26, 2019
View reviewed changes
@smlng smlng dismissed their stale review June 26, 2019 09:24

did not apply

@jia200x jia200x force-pushed the pr/sx127x_netops branch from c6e7cbe to 24527fa Compare June 26, 2019 09:32
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jun 26, 2019

forgot to add netopts to crosslayer/netopt.c. Amended and squashed directly

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 26, 2019

24527fa should be squashed with 2775c8b (no need for a separate commit).

forgot to add netopts to crosslayer/netopt.c. Amended and squashed directly

Ok !

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 26, 2019

There is a problem with 24527fa: it modifies unrelated stuff...

@aabadie
Copy link
Copy Markdown
Contributor

aabadie commented Jun 27, 2019 •
edited
Loading

24527fa commit message is invalid: it references tests/at86rf2xx whereas it touches tests/driver_sx127x

@jia200x jia200x force-pushed the pr/sx127x_netops branch from 24527fa to 77f5863 Compare June 27, 2019 13:36
@jia200x jia200x added the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 27, 2019
@jia200x jia200x force-pushed the pr/sx127x_netops branch from 77f5863 to ef75dac Compare June 27, 2019 13:44
@jia200x jia200x removed the State: waiting for other PR State: The PR requires another PR to be merged first label Jun 27, 2019
aabadie
aabadie reviewed Jun 27, 2019
View reviewed changes
tests/driver_sx127x/main.c Outdated
(void)argv;

netdev_t *netdev = (netdev_t *)&sx127x;
netdev_t *netdev = (netdev_t*) &sx127x;
Copy link
Copy Markdown
Contributor

@aabadie aabadie Jun 27, 2019

Choose a reason for hiding this comment

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

Uncrustify must be run again on this file.

@jia200x jia200x force-pushed the pr/sx127x_netops branch from ef75dac to 8b3d1b6 Compare June 27, 2019 15:11
aabadie
aabadie requested changes Jun 28, 2019
View reviewed changes
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.

I tested this PR and it works. I still have a usability issue the syncword command, see below.

@jia200x jia200x force-pushed the pr/sx127x_netops branch from 8b3d1b6 to 398cf66 Compare June 28, 2019 09:36
@jia200x
Copy link
Copy Markdown
Member Author

jia200x commented Jun 28, 2019

done :)

aabadie
aabadie approved these changes Jun 28, 2019
View reviewed changes
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 and go!

@aabadie aabadie merged commit 6c271f6 into RIOT-OS:master Jun 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aabadie aabadie aabadie approved these changes

+1 more reviewer

@smlng smlng smlng left review comments

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

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jia200x @aabadie @smlng