Skip to content

LoRa support #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 18 commits into from
Jan 16, 2020
Merged

LoRa support #11

merged 18 commits into from
Jan 16, 2020

Conversation

mirkokurt
Copy link
Contributor

@mirkokurt mirkokurt commented Dec 10, 2019

Support for LoRaWAN boards.
Connection handlers have been separated in TcpIP connection handlers and LPWAN connection handlers. For LPWAN protocols, a LoRa connection handler has been implemented. It uses the MKRWAN library: https://github.com/arduino-libraries/MKRWAN

To be used with:
arduino-libraries/ArduinoCloudThing#48
arduino-libraries/ArduinoIoTCloud#83

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

Unfortunately code which has been refactored already was un-refactored during this PR. Furthermore I request the code within LoRaConnectionHandler::update to follow the same pattern as shown in WiFiConnectionHandler::update() (the refactored version) in order to increase readability.

@aentinger
Copy link
Contributor

Hi @mirkokurt 👋
I've seen you have restored the original code of the master branch of the WiFiConnectionHandler::update method. What's missing now is to refactor LoRaConnectionHandler::update in a similiar way as WiFiConnectionHandler::update since this makes the control flow from one state to the next easier to spot.

@mirkokurt
Copy link
Contributor Author

Hi @mirkokurt 👋
I've seen you have restored the original code of the master branch of the WiFiConnectionHandler::update method. What's missing now is to refactor LoRaConnectionHandler::update in a similiar way as WiFiConnectionHandler::update since this makes the control flow from one state to the next easier to spot.

Hi @lxrobotics,
yes, it was a work in progress. Now the LoRaConnectionHandler has been refactored.

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

We are getting closer but there still need to be some changes made 😉

Copy link
Contributor Author

@mirkokurt mirkokurt left a comment

Choose a reason for hiding this comment

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

We are getting closer but there still need to be some changes made 😉

Hi @lxrobotics,
I've made the corrections and refactoring as you asked. I gave a short answer to each of the changes you've requested. Let me know if there are other changes I have to apply.

Copy link
Contributor

@aentinger aentinger left a comment

Choose a reason for hiding this comment

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

We are getting in the right direction - however there have been many formatting changes which enlarges the change set that needs to be reviewed with not benefit. Please keep in mind to not do formatting changes when doing functional changes.

Also, if I mark formatting changes on some occasions I expect you to go through the whole change set and repair it everywhere it happens, not just where I remark on it.

@mirkokurt
Copy link
Contributor Author

We are getting in the right direction - however there have been many formatting changes which enlarges the change set that needs to be reviewed with not benefit. Please keep in mind to not do formatting changes when doing functional changes.

Also, if I mark formatting changes on some occasions I expect you to go through the whole change set and repair it everywhere it happens, not just where I remark on it.

Hi @lxrobotics,

formatting changes have been applied automatically by my astyle code linter when I ran it. Usually I run the linter when I make a big refactoring to correctly format the code I just written. I didn't expect the linter to change the formatting of the code not written by me because I thought that the code was already in the astyle format. Now I restored the previous formatting. To avoid this in the future I propose to make a new standalone commit with only formatting changes to put all the files in full astyle format.

@aentinger
Copy link
Contributor

@mirkokurt Since the PR seems to be complete now - have you tested it with all supported boards (MKR GSM 1400, MKR WiFi 1010 or MKR 1000, MKR WAN and a ESP8266 based board) that everything still works for all boards?

Copy link
Contributor Author

@mirkokurt mirkokurt left a comment

Choose a reason for hiding this comment

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

@mirkokurt Since the PR seems to be complete now - have you tested it with all supported boards (MKR GSM 1400, MKR WiFi 1010 or MKR 1000, MKR WAN and a ESP8266 based board) that everything still works for all boards?

The LoRa support for Arduino IoTCloud implementation has a server counterpart. Tests are ongoing on both client and server side. Not every board in the list have been fully tested at the moment.

@aentinger
Copy link
Contributor

I've got slightly bad news for you. I was asked to merge the NB1500 connection handler PR into the master and have consequently released v0.2.0. Therefore this PR needs to be carefully rebased.

@per1234
Copy link
Contributor

per1234 commented Jan 10, 2020

MKRWAN should be added to the library.properties depends field once this is merged (or ideally as part of this PR).

@mirkokurt
Copy link
Contributor Author

MKRWAN should be added to the library.properties depends field once this is merged (or ideally as part of this PR).

Done

@aentinger aentinger merged commit d405789 into arduino-libraries:master Jan 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants