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

Status page improvements #2289

Merged
merged 4 commits into from
Sep 4, 2021
Merged

Status page improvements #2289

merged 4 commits into from
Sep 4, 2021

Conversation

neocturne
Copy link
Member

This code is usually running on an embedded CPU without FPU. In
addtition to its inefficience, the algorithm is also much harder to
understand.

Replace the logarithm formula with a simple loop.
Do not depend on the respondd-airtime module just to get the configured
channels. This removes the display of the frequency in addition to the
channel, as it is not readily available.

In addition, the translation string is improved to allow for text after
the channel number.
Fixes the display of client counts, which are numbers and not strings
in the respondd data.

Fixes: 3a885a1 ("gluon-status-page: make "gateway nexthop" a link (#2278)")
@AiyionPrime
Copy link
Member

Calling all of the introduced changes improvements is a little subjective.
After this PR the statuspage will no longer reflect channel changes without reloading the page, while it does update all other wifi-info dynamically.
Furthermore it does now as well in the release v2021.1, so you're not just dropping this feature from master, but should reflect in the docs that it will behave differently in upcoming releases, and that that's a necessary deterioration.

@neocturne
Copy link
Member Author

Calling all of the introduced changes improvements is a little subjective.
After this PR the statuspage will no longer reflect channel changes without reloading the page, while it does update all other wifi-info dynamically.

I'm not sure what information you are talking about. Most of the information on the status page is static. We don't have any other wifi info on the status page at the moment except for connection quality details for mesh peers - which are dynamic because they change all the time, unlike the node's channel selection.

Furthermore it does now as well in the release v2021.1, so you're not just dropping this feature from master, but should reflect in the docs that it will behave differently in upcoming releases, and that that's a necessary deterioration.

I intend backport these changes to v2021.1.x, as I consider the existence of the PHY index a bug in the data format, and I don't want anyone to start depending on this value (the PHY index isn't guaranteed to be stable across reboots, and it is fairly meaningless outside of a single node). I agree that the change should be mentioned in the release notes.

@AiyionPrime
Copy link
Member

AiyionPrime commented Aug 9, 2021

One point is the place the channel info is currently announced.
I agree it's not perfect, call it a bug if you want;
would have been nice if you'd pointed that out in april, when you were part of the merge discussion.
I have no objections at all if you have got a better way to announce it.

The other point is we currently have a proper respondd provider for the wifi-channel, that updates dynamically, which makes improving large setups with poor wifi-performance a little less painful.
It might not be your primary intention, but you do quite shortcut by not providing that anymore.
Removing that feature again is not an improvement in my eyes.

While it might not have been obvious, I've put effort in adding all info I wanted in the statuspage via respondd in order to keep the statuspage modular.
If it was my vote, we would not ever add any info to the statuspage again without adding a respondd- or similar provider for it.

@neocturne
Copy link
Member Author

would have been nice if you'd pointed that out in april, when you were part of the merge discussion.

My apologies, I had an extended period of illness in April and May and had to reduce my participation in the discussion.

One point is the place the channel info is currently announced.

Huh, where was this part of the discussion at all? I don't have a problem with the current respondd-airtime module except for the PHY index - but I don't think it is the right approach to use this for the status page, as getting the data statically from ubus avoids the dependency on that module.

The other point is we currently have a proper respondd provider for the wifi-channel, that updates dynamically, which makes improving large setups with poor wifi-performance a little less painful.

You mean when a DFS event comes in an forces a node in outdoor mode to switch its channel? At least that's the only reason I'm aware of that might cause a node to switch a channel at all at runtime. And the outdoor mode channel doesn't even have anything to do with the mesh connections...

For large setups, centralized monitoring (which will continue to have dynamic channel information from the respondd-airtime module) is more appropriate than the status page IMO.

While it might not have been obvious, I've put effort in adding all info I wanted in the statuspage via respondd in order to keep the statuspage modular.
If it was my vote, we would not ever add any info to the statuspage again without adding a respondd- or similar provider for it.

My opinion is the exact opposite: respondd providers should never be added for the sake of the status page. If something needs to be dynamic to be useful, we can add another non-respondd event provider, but keeping things static is usually preferable from a performance point of view. respondd providers are first and foremost to be useful for central monitoring - the status page only uses this data because it's available anyways.

Adding respondd data always involves a tradeoff: Doing so will increase the load of the node, and increase the respondd data packet size. If possible, the data should fit in a single unfragmented packet. It's been a while since I last checked - it's possible that we already hit that threshold. We might want to reduce respondd functionality to improve performance in that case, but of course that will require careful consideration, and we might have to accept the fragmentation...

@AiyionPrime
Copy link
Member

My comments above should be read with caution, as I've bee certainly biased on the matter.

I think Neoraiders reasoning and his intentions are sound. As I implemented the details for the statuspage we were aware the solution was ugly. It just did not occur to me it would be cleaner not to use a respondd provider. Especially in regard to #644 which is tagged with something like "accepted feature".

For our community it was a nice feature to have dynamic updating channel-info, as we are often in environments where several people "optimize" a local network. Nevertheless I do not intend to block this, sorry for not responding.

@neocturne neocturne merged commit 90fe74b into master Sep 4, 2021
@neocturne neocturne deleted the status-page-improvements branch September 4, 2021 19:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants