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

Independent build #2

Merged
merged 8 commits into from
Oct 22, 2021
Merged

Independent build #2

merged 8 commits into from
Oct 22, 2021

Conversation

marvinmarnold
Copy link

@marvinmarnold marvinmarnold commented Oct 14, 2021

Why
The hm-pktfwd build process is confusing. This is one of two proposals on how to structure the Dockerfile in order to improve things. We have decided to split up the build process by repo instead of having a mono-Dockerfile in hm-pktfwd.

How
Builds libloragw only and outputs to /opt/output to be retrieved by packet_forwarder and hm-pktfwd.

References

@marvinmarnold
Copy link
Author

@shawaj thanks for pointing out the original wrong PR location. Both this and NebraLtd/packet_forwarder#2 are meant to be drafts.

@shawaj shawaj marked this pull request as draft October 14, 2021 12:55
@vpetersson
Copy link

LGTM so far structure wise.

@vpetersson
Copy link

vpetersson commented Oct 14, 2021

We should probably use releases here and reference the releases in --from=lora_gateway line in packet_forwarder? Wdyt?

@marvinmarnold marvinmarnold force-pushed the marvinmarnold/independent-build branch 2 times, most recently from 9bcb2b2 to bdea0fb Compare October 18, 2021 15:46
@marvinmarnold marvinmarnold force-pushed the marvinmarnold/independent-build branch from bdea0fb to ff4e229 Compare October 20, 2021 14:47
@marvinmarnold
Copy link
Author

We should probably use releases here and reference the releases in --from=lora_gateway line in packet_forwarder? Wdyt?

Using releases makes sense but I'm not sure how to do that automagically. For the moment, I'm just using commit hashes like we do in helium-miner-software.

@marvinmarnold marvinmarnold marked this pull request as ready for review October 20, 2021 17:56
@marvinmarnold marvinmarnold requested a review from a team October 20, 2021 18:02
@shawaj
Copy link
Member

shawaj commented Oct 20, 2021

We should probably use releases here and reference the releases in --from=lora_gateway line in packet_forwarder? Wdyt?

Using releases makes sense but I'm not sure how to do that automagically. For the moment, I'm just using commit hashes like we do in helium-miner-software.

We can automatically tag and release easily as per

https://github.com/NebraLtd/hm-miner/blob/79923ea6ab73a69c446a99cbad658e6453d97c9d/.github/workflows/check-latest-release.yml#L44-L59

But I guess this would also require a semver action such as:
https://github.com/marketplace/actions/git-semantic-version

Otherwise we would have to manually tag/release.

Maybe easier to just use the commit SHA like you have already?

compile_libloragw_for_spi_bus spidev32766.0
}

copy_reset_script() {
Copy link
Member

Choose a reason for hiding this comment

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

Probably don't need this

Can just use the same reset script as for the sx1302 instead of copying multiple

See also NebraLtd/hm-pktfwd#59 (comment)

Copy link
Author

@marvinmarnold marvinmarnold Oct 21, 2021

Choose a reason for hiding this comment

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

I do see some small differences between the scripts but I'm unclear how significant they are. I agree we should only copy one file if the differences are not important.

  1. sx1301 sets input during init but sx1302 doesn't

  2. sx1301 stop doesn't reset but sx1302 does

Want to make the call @shawaj?

Copy link
Member

Choose a reason for hiding this comment

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

  1. the reason they are doing that is because they seem to be using GPIO7 for both the reset pin and as the SPI chip select pin. I don't understand why they did this - it is a bizarre choice. But we do not need to set any inputs on the reset pin as it is only used for reset and nothing else

  2. iot_sk_init() in sx1301 folder is the same function essentially as init() and reset() together in the sx1302 function. so this is doing the same thing just in separate functions.

So I would say the sx1302 one makes more sense to keep

Copy link
Author

Choose a reason for hiding this comment

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

  1. Cool
  2. @shawaj Not sure we are talking about the same thing. The relevant code is in a case statement to handle start and stop commands differently. I agree that iot_sk_init() vs init()+reset() are functionally the same for start. But for stop, the sx1301 version is missing the call to reset/init all together. That said, I'm OK using the sx1302 code only and will make that change.

Copy link
Member

Choose a reason for hiding this comment

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

@marvinmarnold i don't know exactly why there is a need to reset before the term() command - to be honest I think that is unecesary in sx1302 - but it also will not harm so may as well leave it in (for either sx1301 or sx1302)

@marvinmarnold
Copy link
Author

We can automatically tag and release easily as per

https://github.com/NebraLtd/hm-miner/blob/79923ea6ab73a69c446a99cbad658e6453d97c9d/.github/workflows/check-latest-release.yml#L44-L59

But I guess this would also require a semver action such as: https://github.com/marketplace/actions/git-semantic-version

Otherwise we would have to manually tag/release.

Maybe easier to just use the commit SHA like you have already?

Let's prioritize getting the current work over the line. The steps you laid out should work. Created a new issue to track this work.

This script duplicates logic in the sx1302_hal version.
That version will be used instead.
@marvinmarnold marvinmarnold merged commit 3a181a5 into master Oct 22, 2021
@marvinmarnold marvinmarnold deleted the marvinmarnold/independent-build branch October 22, 2021 21:11
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.

3 participants