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

LoRa WAN US915 Support #603

Closed
wants to merge 4 commits into from
Closed

Conversation

mirackara
Copy link
Contributor

Adds a driver for us 915 protocol to the lora WAN drivers.

mirackara and others added 2 commits September 26, 2023 17:02
Adds a driver for us 915 protocol to the lora WAN drivers.
@iamemilio
Copy link
Contributor

mirackara#1

fix lora us915 logic to step into 500 kHz range
@iamemilio
Copy link
Contributor

@deadprogram should be good to go now, sorry I didn't get to test it. I got food poisoning 💀

@iamemilio
Copy link
Contributor

Note that because this re-structures the implementation of this driver, it will be breaking for #535

@deadprogram deadprogram changed the base branch from release to dev October 5, 2023 08:03
@deadprogram
Copy link
Member

Here is an example where changing to Channel interface using functions instead of struct with values is a problem: https://github.com/hybridgroup/tinyglobo/blob/main/main.go#L25-L28

@deadprogram
Copy link
Member

The most straightforward solution to the TinyGlobo example within how this PR works, would be to provide SetXXX() functions for each of the values.

Example:

func (c *ChannelAU) SetFrequency(freq uint32)  error { 
    c.frequency = freq
    return nil
}

There is some duplication of code, but it would allow for validation to be added at some point.

What do you think @mirackara @iamemilio ?

@iamemilio
Copy link
Contributor

I like that. It feels like a much safer way to handle this data structure.

@deadprogram
Copy link
Member

@iamemilio can you add to this PR please? 😸

Copy link
Contributor

@soypat soypat left a comment

Choose a reason for hiding this comment

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

Observed maybe a lot of code could be simplified a bunch, left comments

@@ -7,27 +7,56 @@ const (
AU915_DEFAULT_TX_POWER_DBM = 20
)

type ChannelAU struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks like identical code for all ChannelXX types-
Should we have a base channel type and compose these "higher" level types with it?

Copy link
Contributor

Choose a reason for hiding this comment

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

ie:

type channel struct {
	frequency       uint32
	bandwidth       uint8
	spreadingFactor uint8
	codingRate      uint8
	preambleLength  uint16
	txPowerDBm      int8
}
// Implement methods for channel here...

type ChannelAU struct {
     channel // Exposes exported channel methods.
}

Copy link
Member

Choose a reason for hiding this comment

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

@soypat that sounds like an excellent refactoring for a subsequent PR.

Copy link
Member

Choose a reason for hiding this comment

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

I will submit a PR that does exactly this later on...

Copy link
Member

Choose a reason for hiding this comment

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

See #611

@deadprogram
Copy link
Member

Forgot to mention that I tested this in EU868 and it still works. As long as the US915 region is working, then I suggest we merge and then can do some additional refactoring from there.

@deadprogram
Copy link
Member

Commits picked up in #611 so now closing this PR. Thank you very much everyone!

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.

4 participants