Skip to content

define new Regions AS923_1B and CD900_1A #250

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

Merged
merged 5 commits into from
May 16, 2022
Merged

define new Regions AS923_1B and CD900_1A #250

merged 5 commits into from
May 16, 2022

Conversation

mikev
Copy link
Contributor

@mikev mikev commented May 13, 2022

AS923_1B defines a Region supporting Malaysia. It is illegal in Malaysia to use any frequency above 924 Mhz, hence the reason for this new Region. The AS923_1B defines Uplink frequencies at [922.0, 922.2, 922.4, 922.6, 922.8, 923.0, 923.2, 923.4]

CD900_1A defines a new testnet Region. This will be used for testing new Region parameters. It is centered in the ocean at the Challenger Deep which is the lowest sea floor depth on Earth - https://en.wikipedia.org/wiki/Challenger_Deep

CN779 is officially deprecated by the LoRa Alliance and should never be used.

PR is pending to update the proto Region enum
helium/proto#132

Note, this change updates the Cargo.lock link to Helium.Proto. I'll let madninga decide when this is OK to merge. It does include several updates to Helium.Proto.

@mikev mikev self-assigned this May 13, 2022
@mikev mikev requested review from madninja and lthiery May 13, 2022 23:36
Copy link
Member

@madninja madninja left a comment

Choose a reason for hiding this comment

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

You can't just move the CN799 enum entry right? It's still going to be defined in, and the constant still needs to be there even if it's no longer used

@oberchoo
Copy link

Yes, Malaysia will need that in making Helium Network legal.

@mikev mikev requested a review from vihu May 16, 2022 19:17
@mikev
Copy link
Contributor Author

mikev commented May 16, 2022

I've added back CN779.

I do believe we should sort out an update method to permanently remove some enum names. Ideally we are going to rename some of these plan names to be more explanatory and less vague. For example, US915 will become US915_SB2. This will allow us to deploy any variant such as US915_SB1, US915_SB7, etc. So, I'd like to see some method to be able to permanently remove names, otherwise we'll need to leave US915 in place forever which will be confusing.

@mikev mikev marked this pull request as ready for review May 16, 2022 20:55
@mikev mikev requested a review from madninja May 16, 2022 20:55
@madninja
Copy link
Member

I've added back CN779.

I do believe we should sort out an update method to permanently remove some enum names. Ideally we are going to rename some of these plan names to be more explanatory and less vague. For example, US915 will become US915_SB2. This will allow us to deploy any variant such as US915_SB1, US915_SB7, etc. So, I'd like to see some method to be able to permanently remove names, otherwise we'll need to leave US915 in place forever which will be confusing.

For backwards compatibility you can't really do that without a synchronized first upgrade where you break every one which is something we'd avoid. The name can be changed however since that won't break the on the wire format, just the source builds.

So we can rename US915 to US915_SB2 in the proto and it wouldn't break running systems with different older and newer versions of the proto

@madninja madninja merged commit 0b1b68f into main May 16, 2022
@madninja madninja deleted the mv/as923_1b branch May 16, 2022 21:24
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