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.

Sign up for GitHub

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

Jump to bottom

lora: Initial support for sensor example #344

Merged
merged 1 commit into from
Jan 12, 2024
Merged

lora: Initial support for sensor example #344

merged 1 commit into from
Jan 12, 2024

Conversation

alistair23
Copy link
Contributor

No description provided.

ppannuto
ppannuto requested changes Aug 30, 2023
View reviewed changes
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

Super cool to have a LoRa example!!

examples/lora/build-RadioLib.sh Outdated Show resolved Hide resolved
examples/lora/build-RadioLib.sh Show resolved Hide resolved
examples/lora/sensor-transmit/main.cc Outdated Show resolved Hide resolved
@alistair23
Copy link
Contributor Author

I think I have addressed all of the issues

bradjc
bradjc reviewed Sep 6, 2023
View reviewed changes
Copy link
Contributor

@bradjc bradjc left a comment

Choose a reason for hiding this comment

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

I don't like submodules, so why not just commit the built libraries and relevant headers? That makes it easy for new users.

examples/lora/sensor-receive/Makefile Show resolved Hide resolved
examples/lora/sensor-transmit/Makefile Outdated Show resolved Hide resolved
examples/lora/sensor-receive/Makefile Outdated Show resolved Hide resolved
@alistair23
Copy link
Contributor Author

I don't like submodules, so why not just commit the built libraries and relevant headers? That makes it easy for new users.

I don't think we should do that. RadioLib is constantly adding new features, such as the work-in-progress LoRaWAN support.

If we check in binaries they are going to become old and stale and make it difficult for users to add features, get updates or make changes. The Makefile will automatically build RadioLib so it should be easy for new users to get started, while still allowing them access to the RadioLib libraries. Just running make is all it takes to build these examples

As for the submodule, I understand you don't like them. But submodules are easier then having weird Makefile scripts to clone things. It is what we already do for lua

@alistair23 alistair23 force-pushed the alistair/radio-lib branch 3 times, most recently from 5adb9a8 to 4505330 Compare September 7, 2023 10:41
@alistair23
Copy link
Contributor Author

Urgh! Except that it doens't build in the CI. It looks like the cmake doesn't like the toolchain location

@alistair23 alistair23 force-pushed the alistair/radio-lib branch 2 times, most recently from b5d36ac to 387ebc9 Compare September 11, 2023 01:07
@alistair23
lora: Initial support for sensor example
4a1a816 
Signed-off-by: Alistair Francis <alistair@alistair23.me>
@alistair23
Copy link
Contributor Author

CI is all fixed, this is ready to go again

ppannuto
ppannuto approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@ppannuto ppannuto left a comment

Choose a reason for hiding this comment

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

🎉

@alistair23
Copy link
Contributor Author

Ping!

@alistair23
Copy link
Contributor Author

Ping

@alistair23
Copy link
Contributor Author

Ping!

@alevy alevy added this pull request to the merge queue Jan 12, 2024
Merged via the queue into tock:master with commit f035329 Jan 12, 2024
2 checks passed
@alistair23 alistair23 deleted the alistair/radio-lib branch January 15, 2024 00:03
tyler-potyondy pushed a commit to tyler-potyondy/libtock-c that referenced this pull request Mar 13, 2024
@alevy
Merge pull request tock#344 from alistair23/alistair/radio-lib
fd6ed89 
lora: Initial support for sensor example
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brghena brghena brghena left review comments

@bradjc bradjc bradjc left review comments

@ppannuto ppannuto ppannuto approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alistair23 @brghena @ppannuto @bradjc @alevy