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

Fixe complier warnings and missing AT return states #35

Open
wants to merge 28 commits into
base: master
Choose a base branch
from

Conversation

flhofer
Copy link

@flhofer flhofer commented Apr 28, 2021

This merge addresses two issues:

When compiling the firmware I noted missing function declarations and includes. Fixed, added them in lora.h, main.c and sx1276.h`

Added also the missing +OK= AT responses for the getters in ATslave in at.c (+RSSI, +SNR...)

@facchinm
Copy link
Member

Hi @flhofer ,
thank you very much for the submissions! The changes look very good to me, both here and arduino-libraries/MKRWAN#93 .
As a release method, I'd prefer to advance the version number to 1.2.4 so we are aware of the changes, then update MKRWAN library and updater accordingly. How does it sound?

@flhofer
Copy link
Author

flhofer commented Apr 28, 2021

Sure!

A thing I was unsure about is the meaning of master-1.3.1 and the experimental-1.4 branches. Am I correct that those contain the updated ST middleware only?
As a next thing I would like to address the AT responses format to be really V250 compliant and the parsing of the return values in the library to not mismatch +ERR for +ERRxxx as it does now. However, that may cause compatibility issues between FW and library.
I have 8 of these boards lying around here and about a month of time to get the most out of them..

@aalbinati
Copy link

Hi @flhofer,

Thanks! Less than 24hs in the community and already making big improvements! Do you think it would be possible to update the firmware to LoRaWAN 1.1? I think that would be great.

remove duplicate include main.cpp
@CLAassistant
Copy link

CLAassistant commented Apr 28, 2021

CLA assistant check
All committers have signed the CLA.

@flhofer
Copy link
Author

flhofer commented Apr 28, 2021

@facchinm ver number update done
@aalbinati for the update to 1.1, the thing is that LoraWan 1.0.x and 1.1 are incompatible. From what I know the newer version uses a different session key scheme and can thus not be used on older systems. While many Network servers understand both, the devices might have to choose between A or B.

I didn't deep-dive into the different ST middleware versions yet, but checking the commissioning template it seems that the experimental 1.4 tries to address LoRaWan 1.1.
We might want to consider two separate branches/dev-lines and an adapted ATslave using the same AT commands so as to use the same MKRWAN library for both. What do you think?

@aalbinati
Copy link

I agree with you, that would be the best option. The library could then identify which version is being used with a AT+VER command and change its behavior accordingly.

@flhofer
Copy link
Author

flhofer commented Apr 29, 2021

@aalbinati Fine for me
I implemented some Changes for the MKRWan library now to discern +ERR from +ERRxxx with some optimizations. I also have an Idea for how to implement asynchronous calls. Once we change the firmware to respect AT standard and respond with +VARIABLE:<value> we can use a poll function that returns value type and value. However, to keep things simple we might use that only for send/receive and not for setup commands.

@aalbinati
Copy link

That's great. We should also try to fix the OTAA activation which isn't working with the latest changes and the we will be good to create a new branch or use the experimental 1.4 one to implement LoRaWAN 1.1. Or there are other improvements to be made beforehand?

@flhofer
Copy link
Author

flhofer commented Apr 29, 2021

OK, so now I went through the code again and formatted both with +<CMD>=<VALUE> responses. The library autodetects which one to use. See the library pull for all changes (there are a lot!)
Anyway, wandering through code I saw a mistake in the receive function using +RECV also for binary instead of +RECVB.
With the new parser in the library now also Binary receive should be possible, although limited to 64 bytes.

Did a long set of tests and all seems fine so far. I thought to push to see if someone might want to join :)
Tomorrow I will continue on the live network, 🤞

@flhofer
Copy link
Author

flhofer commented Aug 24, 2021

Hi @aalbinati @facchinm
I finished my experiments and added some final features for the binary send.
The last commit adds a static send buffer (otherwise it had to allocate 500 bytes of stack on every call) and the send buffer is scanned for AT_ERROR_RX_CHAR passed by vcom) (see #40)
Although I didn't find the reason for #40, it is something on the lower level too complicated to fix quickly and easily. I think we are done here. What do you say?

Florian

README.md Outdated Show resolved Hide resolved
Co-authored-by: mactkg <[email protected]>
@flhofer
Copy link
Author

flhofer commented May 4, 2022

@mactkg Can we merge and close this? It's stalled for a while now..
Would love to end this and go on with the new 1.3..
Thanks

@mactkg
Copy link

mactkg commented May 4, 2022

@flhofer
Sure! Thanks for update!(I'm out now, so I can't update status of my review)

@flhofer
Copy link
Author

flhofer commented May 19, 2022

@mactkg Ping

@mactkg
Copy link

mactkg commented May 20, 2022

@flhofer I'm just user of mkrwan1300-fw, not committer or maintainer of this repo. Therefore, I can't approve this PR and I think this PR should be reviewed by @aalbinati or @facchinm.

@aalbinati
Copy link

I'm no maintainer either. @facchinm is the indicated to approve the PR.

@flhofer
Copy link
Author

flhofer commented May 27, 2022

I see. So both repositories FW and library depend on his approval. Understand.

@facchinm

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.

5 participants