search

TheThingsNetwork / arduino-device-lib

Arduino Library for TTN Devices
MIT License
207 stars 96 forks source link

Fixed LoRa module not waked from sleep by autoBaud sometimes #231

Closed hallard closed 6 years ago

hallard commented 6 years ago

I increased the break time condition setting up serial port to lower baud rate when writing 0 (break) on wake method Bug could be reproduced by non stop shaking the node, with this code, seem fixed

May be we could also arrange autoBaud to reflect this change and do only one method.

johanstokking commented 6 years ago

Can you file a PR in https://github.com/TheThingsProducts/arduino for the board file @samuel-puschacher ?

hallard commented 6 years ago

@Johan, Why you don't just Fork Samuel board repo since he's the creator and the maintainer? I let you see with him how to organize this because it's not on any of my repo.

johanstokking commented 6 years ago

@hallard @samuel-puschacher I'm fine with forking if the repository is named arduino or arduino-boards

samuel-puschacher commented 6 years ago

@johanstokking Done

johanstokking commented 6 years ago

@samuel-puschacher I cannot fork the repository as it is now:

  1. The commit trail is very dirty
  2. The URLs should be on github.com/TheThingsProducts/... We can fork it and change it, but see [1]
  3. GitHub has tags for versioning and releases for distribution, see for example the Arduino library. We can fork it and do our own tags and releases, but then this repository has to be clean, and see also [1]
  4. The product is called The Things Uno (no caps)

[1] The point of forking is to diverge from an existing project and/or contribute changes back to the original repository. Are you planning on maintaining this repository in your account, regardless of what is in TheThingsProducts (i.e. diverging projects)? If not, why not create a PR on the existing repository?

hallard commented 6 years ago

@johanstokking When I first forked the library, the code was already containing some sprintf() in v2.5.1. It was done by this commit So when I optimized the code size, I won approx 700 bytes of flash, and yes I added sprintf() but it was not new using this call since some where already in place on original code. I don't see what I've done wrong on this point. Charles

johanstokking commented 6 years ago

Indeed, it did contain sprintf(), but I'd rather get rid of it than introducing more occurrences, especially in unrelated feature branches like these.

jpmeijers commented 6 years ago

@hallard @samuel-puschacher and me discussed this PR over the weekend. We think it would be better splitting this PR up into smaller ones. I was planning to do this, but didn't get time for it yet. But the idea is to have the sleep fix in a different PR than the code size optimisation. This also means the sprintf's will be in a different PR, where we can optimise them out in a better way.

When the PR split is done we can close this one.

jpmeijers commented 6 years ago

Merging this into a separate branch to do the splitting.