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

Migrate frequency band suffix options to uci sections for each band. #896

Merged

Conversation

germanferrero
Copy link
Contributor

@germanferrero germanferrero commented Sep 22, 2021

While working on a service that would expose ubus enpoints for changing AP settings (such as disabling ap mode), I found out that it is really difficult to work with list mode option when the frequency suffix is used.
A lot of thinking and code is needed to unpack the configuration scenario, handle all corner cases and pack it again when doing some change programmatically (or even manually).
This PR proposal is to migrate from configs like these:

config lime 'wifi'
    list modes 'ap'
    list modes 'apname'
    list modes 'ieee80211s_5ghz'
    option htmode_5ghz '40'

To

config lime-wifi-band '5ghz'
    list modes 'ap'
    list modes 'apname'
    list modes 'ieee80211s'
    option htmode '40'

config lime-wifi-band '2ghz'
    list modes 'ap'
    list modes 'apname'

In this way, disabling ap mode from 2ghz requires only to remove ap mode from 2ghz.modes list
Otherwise we need to think of replacing 'ap' with 'ap_5ghz' in wifi.modes to avoid modifying 5ghz radios behavior too.

The code for wireless.configure gets simplified aswell.
Now all options for a radio would be taken from three levels, in order of precedence: radio (config wifi), band (config lime-wifi-band), general (config lime wifi).

A migration script is added as a uci-default so lime-community and lime-node configurations out there are transformed to the new syntaxis before running lime-config on first boot.
There is a test for the migration script as well, and I've done manual testing too. But please review it carefully.

Breaking changes

  • Drops support for "<option_name>_<frequency_suffix>" on specific radio configs. This is a no sense config, as one radio always corresponds to one frequency band. (I'll like to hear from you if you consider that this should still be supported).

Details:

  • lime.wifi.modes support is kept to avoid duplication of modes when no differentiation between bands is needed.

@codecov-commenter
Copy link

codecov-commenter commented Sep 22, 2021

Codecov Report

Merging #896 (e199852) into master (e133d60) will increase coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head e199852 differs from pull request most recent head 9e9a748. Consider uploading reports for the commit 9e9a748 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #896      +/-   ##
==========================================
+ Coverage   74.51%   74.66%   +0.15%     
==========================================
  Files          42       43       +1     
  Lines        3629     3659      +30     
==========================================
+ Hits         2704     2732      +28     
- Misses        925      927       +2     
Impacted Files Coverage Δ
...es/lime-system/files/usr/lib/lua/lime/wireless.lua 97.39% <100.00%> (-0.32%) ⬇️
packages/pirania/tests/pirania_test_utils.lua 100.00% <100.00%> (ø)
...ges/pirania/files/usr/lib/lua/voucher/vouchera.lua 53.23% <0.00%> (+0.57%) ⬆️
...ckages/pirania/files/usr/lib/lua/voucher/store.lua 96.00% <0.00%> (+4.00%) ⬆️
...ime-proto-wan/files/usr/lib/lua/lime/proto/wan.lua 93.87% <0.00%> (+15.75%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e133d60...9e9a748. Read the comment docs.

@G10h4ck
Copy link
Member

G10h4ck commented Oct 8, 2021

I agree with the change, I was bothered with supporting the frequency suffix there in the first place, as I think that at that point of customization a specific section would be the way to go.

Is the commit "mesh-blocker: add-package" 4e0b122 intentionally mixed into this PR?

@altergui
Copy link
Member

altergui commented Oct 8, 2021

I have no opinion at all on the proposal (since I haven't been following development) but regarding this:

as one radio always corresponds to one frequency band.

just wanted to point out, this is not always the case. I remember using dualband radios where the same radio has both frequency bands (they can't be used concurrently). I remember one USB dongle that worked like this.
In the beginning, routers with two independent radios (one for each band) where marketed as "simultaneous dual-band" to differentiate them from single-radio dualband (which meant "non-simultaneous dual-band")
I guess such single-radio dualband are rare nowadays and can be left unsupported.

@germanferrero germanferrero force-pushed the freq-suffix-options-to-uci-sections branch from 9e9a748 to 40a56e9 Compare October 18, 2021 18:37
@germanferrero
Copy link
Contributor Author

Is the commit "mesh-blocker: add-package" 4e0b122 intentionally mixed into this PR?

It was added by mistake, I've removed it from the PR :)

@germanferrero
Copy link
Contributor Author

just wanted to point out, this is not always the case. I remember using dualband radios where the same radio has both frequency bands (they can't be used concurrently). I remember one USB dongle that worked like this. In the beginning, routers with two independent radios (one for each band) where marketed as "simultaneous dual-band" to differentiate them from single-radio dualband (which meant "non-simultaneous dual-band") I guess such single-radio dualband are rare nowadays and can be left unsupported.

Oh, I didn't know, that's curious.. I'd say that we can go forward with this config schema and address the support for these kind of radios in the future if anyone claim the need for it.

@spiccinini
Copy link
Contributor

Wow, outstanding work @germanferrero !! Contrats!!

@spiccinini spiccinini merged commit 608da03 into libremesh:master Oct 19, 2021
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.

None yet

5 participants