Conversation
* fixed RN2xxx not waked from sleep by autoBaud sometimes * changes requested by johan * changes requested by johan * johan request * cosmetic * HAL abstration of modemStream * Ability to use AltSoftSerial Library * Code optimization, added ~700 bytes flash for sketch * HardwareSerial only used on TheThings Node and Uno * Applied changes requested by johan * Fixed dual check * cosmetic * Added Hardware Serial only for the things node
|
|
|
Thanks for splitting @jpmeijers , looks good to me |
| return readLine(buffer, size); | ||
| } | ||
|
|
||
| size_t TheThingsNetwork::checkModuleAvailable() |
There was a problem hiding this comment.
Having a function like this is a good idea. I would however like to change it to check the version of the RNxxxx module and return a value from an enum stating which hardware and firmware version it is.
We can then use the result to check before we set the frequency plan, and prevent errors when setting the wrong FP into the wrong module.
| return !baudDetermined; | ||
| } | ||
|
|
||
| void TheThingsNetwork::wake() |
There was a problem hiding this comment.
There isn't much difference between the wake() and autoBaud() functions. I would recommend we merge these two.
There was a problem hiding this comment.
Yeah, I do not wanted to change your original one, but I prefer also to merge both, much cleaner
|
|
||
| bool TheThingsNetwork::isSleeping() | ||
| { | ||
| return !baudDetermined; |
There was a problem hiding this comment.
I think we should rename this flag to something like moduleSleeping.
| { | ||
| autoBaud(); | ||
| // to be determined back on wake up | ||
| baudDetermined = false; |
There was a problem hiding this comment.
Again, moduleSleeping would make more sense here.
| delay(100); | ||
| clearReadBuffer(); | ||
| modemStream->setTimeout(10000); | ||
| baudDetermined = true; |
There was a problem hiding this comment.
I actually don't know why we added this flag to start with. Presumably to do a check before every sendCommand call and autoBaud if we didn't do so yet. If we do decide to implement this, we need to write the autoBaud and checkModuleAvailable functions not to use sendCommand.
But for now maybe let's just scrap the baudDetermined flag and rather introduce a moduleSleeping flag.
There was a problem hiding this comment.
I think we need to know and distinct 2 cases
- module available (physically connected and responding) if it's not we do nothing (no send)
- module sleeping, we saw it at init (so available flag set), we put in in sleep (and we know that) but if it does not answer back, so trying to wake it up before sending any data, and check it answer
There was a problem hiding this comment.
Good point. Maybe not a flag then but an enum. Something like this perhaps? Please suggest naming.
enum state {
UNKNOWN,
AWAKE,
SLEEPING
}
There was a problem hiding this comment.
sounds good, may be UNKNOWN => NOT_FOUND or NONE or MISSING
|
@hallard can you address @jpmeijers review comments? |
|
Hmm, you're approving but I don't see @jpmeijers suggestions implemented. |
|
Well, I'm not an expert of git, @jpmeijers splitted my PR into smaller one and created this one, I thought he was asking for validation, not for implementation. And I'm not sure how to do file changes on a PR from another user. |
|
Yes, I'll make the improvements and come back for feedback. Won't happen that fast though. Perhaps the weekend. |
| } | ||
|
|
||
| TheThingsNetwork::TheThingsNetwork(Stream &modemStream, Stream &debugStream, ttn_fp_t fp, uint8_t sf, uint8_t fsb) | ||
| TheThingsNetwork::TheThingsNetwork(SerialType &modemStream, Stream &debugStream, ttn_fp_t fp, uint8_t sf, uint8_t fsb) |
There was a problem hiding this comment.
- Random drive by comment but don't forget to update the documentation since the
Streamis changing to aSerialType. - Wouldn't it make sense to change the variable name from
modemStreamtomodemSerialor something to avoid confusion?
|
@cimm you may want to assist @jpmeijers if wanted! |
|
@johanstokking Happy to help but... I am a software developer, not (yet) a hardware guy. I can see Happy to do the work if someone can give me some simple beginner tasks. |
5 participants
Let's discuss this first before merging.