Skip to content
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

LoRaWAN code in separate class (or repo?) #470

Open
TD-er opened this issue Oct 14, 2019 · 14 comments
Open

LoRaWAN code in separate class (or repo?) #470

TD-er opened this issue Oct 14, 2019 · 14 comments
Labels
enhancement New feature or request

Comments

@TD-er
Copy link
Contributor

TD-er commented Oct 14, 2019

I was looking to integrate parts of your Lorawan code into my own project (ESPEasy) and I came across a few small issues.

First to make it clear where I borrowed the code from (the credentials part), I would like to have the lorawan.h/.cpp files as much the same as you have.
It also would allow me to later merge fixes you have and it would make my own code a lot more clean.

Still it looks better to have it in one C++ object, so I started wrapping it into a class to have it even more contained.
This lead to some compiler issues which may also be indicative of latent problems you may encounter later on.

For example:
Some functions -like the callback functions- need to be static in order to have them set as callback function.
You don't have that compiler/linker issue right now, since your functions are declared in global scope.
Simply adding static to the function declarations in the header file does stop the compiler from complaining, but that's quite dangerous if the header file will be included more than once.

Does it sound OK to you if I would propose a PR to have the lorawan code in a separate class?
This class would then be responsible for creating the queue, lmic task and handling all lora traffic.

I did notice you were also working on the Lora part, so multiple big changes on the same code at the same time is maybe not the most practical :)
Did you plan to work on the lorawan part for a longer period now?

@cyberman54
Copy link
Owner

cyberman54 commented Oct 14, 2019

@TD-er Your PR as stated above would be highly welcome, many thanks!
Time has now come to do it, since i just yesterday released the v1.9.6 package with many code edits. There will now be some time to keep this freezed, so this is the chance to move to your new c++ class.

While refactoring lorawan.cpp please have in mind that my goal is to fully drop integration with "old" LMIC code (maintained by Matthijs Kooijman) and move to the APIs] created in LMIC library by Terrill Moore (MCCI). This is why i added callback functions for RX-, TX- and event handler.

/cc @FlorianLudwig @spmrider @mariusgripp

@TD-er
Copy link
Contributor Author

TD-er commented Oct 14, 2019

I have been looking at the code a bit more today and I think we need also to think of how to handle incoming commands.
Do you prefer a command queue and poll it, or use a callback function?
My preference would be to handle a queue and poll it, so code execution can be divided in as much small chunks as possible.
Also the returned commands or replies from LoRa do not require immediate action.
Or is there an example where real-time action is needed?

@cyberman54
Copy link
Owner

cyberman54 commented Oct 14, 2019

Yes, we should have a queue for incoming commands.
The queue could have a priority mechanism, as it is impemented for the LoRa send queue. So we keep the door open to handle incoming commands in a specific order.
Instead of polling the queue we should use the queue API of RTOS, i.e. xQueueSend and xQueueReceive. This enables RTOS to block the task until an item is received to process, thus keeps the CPU free from polling work.

@TD-er
Copy link
Contributor Author

TD-er commented Oct 14, 2019

I was thinking about using the same type of queue you also used for sending out messages.
So we can also use the same priority code you already had for sending.

@cyberman54 cyberman54 added the enhancement New feature or request label Oct 14, 2019
@cyberman54
Copy link
Owner

So we can also use the same priority code you already had for sending.

Maybe add command priorities to the simple rcommand interpreter by enhancing it's command table.

@TD-er
Copy link
Contributor Author

TD-er commented Oct 14, 2019

That's a good one to have a look.

@cyberman54
Copy link
Owner

@TD-er any news here?

@TD-er
Copy link
Contributor Author

TD-er commented Nov 19, 2019

@TD-er any news here?

Sorry, the last weeks were really really hectic here (on personal level).
So there is some code, but it is not usable right now.
Also the next 2 weeks are quite full, so if you have some things you want/need to do, please do and I will be ready to continue on the LoRa stuff again in December.

@cyberman54
Copy link
Owner

Nothing is due for lorawan.cpp, so no time to hurry.

@cyberman54
Copy link
Owner

@TD-er would it be a solution to migrate the LMIC code in lorawan.cpp to MCCI Arduino Library ?

@TD-er
Copy link
Contributor Author

TD-er commented Nov 24, 2019

I think so, yes.
To me it is still not 100% clear what repository of LMIC has the best user experience both in stability and wrapping API.
But this one does look like it tries to address just that.

@cyberman54
Copy link
Owner

@TD-er any news / plans here?

@TD-er
Copy link
Contributor Author

TD-er commented Jan 14, 2020

News is that I got moved last week to a new house, which did take way way way more time than I hoped it would.
That's also what stalled any development even on my own project.
Plans have not changed, but the available time was a lot less than anticipated.

@cyberman54
Copy link
Owner

@TD-er Time goes by... - still any thoughts on this?
Meanwhile i moved code of this project to MCCI LMIC, and all Lorawan related stuff is distinctly found in lorawan.cpp; but still in old C sytle, not C++.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants