ltq-vdsl-app: extend ubus with dsl statistics

[turboproc]

This pull requests introduces an extension the the ubus dsl integration with a command to expose the DSL statistics data required to render the dsl charts. Statistics include bit-allocation, SNR, QLN and HLOG data.

With this extension ubus calls will look like

root:~# ubus -v list dsl
'dsl' @88f73b6b
        "metrics":{}
        "statistics":{}

Output of ubus call dsl statistics looks like (trailer):

{
        "bits": {
                "downstream": {
                        "groups": 4096,
                        "data": [
                                0,
                                0,
                                0,
                                0,
                                0,
                                0,

This total output is a kind of long but follows similar structure for upstream as for downstream and continues with SNR, QLN and HLog data.

[dhewg]

Now that I mentioned dB, the current m_db function doesn't take invalid values into account. Do you mind fixing that with an additional commit for this PR?

Something like

static inline void m_db(const char *id, int value, int invalid) {
	if (value != invalid)
		m_double(id, (double)value / 10);
}

and then

	// invalid value indicators taken from drv_dsl_cpe_api_g997.h
	m_db("latn", out.data.LATN, 1271);
	m_db("satn", out.data.SATN, 1271);
	m_db("snr", out.data.SNR, -641);
	m_db("actps", out.data.ACTPS, -901);
	m_db("actatp", out.data.ACTATP, -512);

[turboproc]

@dhewg, thnx for your feedback. Will work through item by item. Generally, it is probably more convenient to do the translation into real values in this part rather than presenting an internal representation. Will also simplify the graph rendering part.

[turboproc]

Now that I mentioned dB, the current m_db function doesn't take invalid values into account. Do you mind fixing that with an additional commit for this PR?

Something like

static inline void m_db(const char *id, int value, int invalid) {
	if (value != invalid)
		m_double(id, (double)value / 10);
}

and then

	// invalid value indicators taken from drv_dsl_cpe_api_g997.h
	m_db("latn", out.data.LATN, 1271);
	m_db("satn", out.data.SATN, 1271);
	m_db("snr", out.data.SNR, -641);
	m_db("actps", out.data.ACTPS, -901);
	m_db("actatp", out.data.ACTATP, -512);

Need to cook on this. Still need a mechanism to indicate error values.

[dhewg]

Need to cook on this. Still need a mechanism to indicate error values.

As mentioned above: The snipped I posted intentionally skips invalid values. That's the indicator, there's no need for special values

[janh]

I think this should include a version bump for ltq-adsl-app, as that package copies dsl_cpe_ubus.c from ltq-vdsl-app.

[dhewg]

I took a stab at getting invalid values to output as null. This works for me and includes various cleanups and fixes too: https://github.com/dhewg/openwrt/commits/dsl-ubus-stats

[turboproc]

@feckert , fixed the NAN issue by making a change in m_double. In case of a NAN value it will introduce a "none" value. Having it in m_double makes it as transparent as possible.

[feckert]

@turboproc Something is wrong with the pullrequest, unfortunately I can't test it! Please make one or more commits to your change. Because it will go into the repository when it is merged. One commit for each logical change , so that it is comprehensible what you have done.

Edit: Did you do something! Now it works suddenly. Something is wrong with github!!!

[dhewg]

As mentioned, I already splitted the commits logically and even fixed a few issues, see above. I even provided instructions on how to just use those, any issue with just doing that?

[feckert]

@dhewg but @turboproc pushed also a merge commit https://github.com/openwrt/openwrt/pull/10420/commits this must be changed before merge

[dhewg]

Indeed, which is why I also posted https://github.com/openwrt/openwrt/pull/10420#discussion_r962563806 That'll magically get rid of all problems ;)

[feckert]

I would suggest we merge this now so we can move forward. If something doesn't fit, we can still change it until the next release. For me it currently looks like this. Ubus output: statistics.txt Luci view:

[turboproc]

I would suggest we merge this now so we can move forward. If something doesn't fit, we can still change it until the next release. For me it currently looks like this. Ubus output: statistics.txt Luci view:

Sounds like a plan. For now I wouldn't know what to improve further. Obviously you used the updated Luci module.

[dhewg]

I honestly don't know what the issue is... you don't even have to fix it yourself, I already did. I fixed a pointer error, multiple whitespace issues and got rid of the unnecessary mei fd. There's no work involved, just use it and push it?

[feckert]

@hauke could you please merge this so we could get on and so get more testers on the master branch.

[hauke]

The version from this pull request has a merge commit in it. Please remove the merge commit. Do you prefer this version https://github.com/dhewg/openwrt/commits/dsl-ubus-stats ?

[feckert]

@turboproc package was renamed to ltq-vdsl-vr9-app could you please rebase your changes

Edit: And merge this into one commit ltq-vdsl-app: extend ubus call to provide DSL statistics

[turboproc]

@turboproc package was renamed to ltq-vdsl-vr9-app could you please rebase your changes

Edit: And merge this into one commit ltq-vdsl-app: extend ubus call to provide DSL statistics

Sure, let me check this weekend. Kind of busy these days.

[dhewg]

To prevent this from bitrot, let's please just merge my clean up version at https://github.com/dhewg/openwrt/commits/dsl-ubus-stats

@turboproc already refered that version: https://github.com/openwrt/luci/pull/5895#issuecomment-1279768668

And it's tested on VR9 too, see the follow up comments there.

[feckert]

Either @turboproc updates the pullrequest with the suggested changes from @dhewg or @dhewg opens a new one. Otherwise we won't get anywhere here. Only when this has been merged we can also merge the luci app.

[dhewg]

Agreed in general, but please go ahead and open new PRs if that's what it takes :) It's just annoying when review comments get ignored, and I feel I've invested enough time on this.

[feckert]

Agreed in general, but please go ahead and open new PRs if that's what it takes :) It's just annoying when review comments get ignored, and I feel I've invested enough time on this.

I can understand your point. But we were almost done.

For the feature: @dhewg Is it really that much work to open a new pullrequest with the two changes from you? @turboproc Couldn`t you have update your pullrequest with the two changes from @dhewg?

Since I would like to see the feature in OpenWrt, I took the feature from @turboproc and the changes @dhewg and created a new pullrequest

Anyway thank you for your work :+1:

[turboproc]

Agreed in general, but please go ahead and open new PRs if that's what it takes :) It's just annoying when review comments get ignored, and I feel I've invested enough time on this.

I can understand your point. But we were almost done.

For the feature: @dhewg Is it really that much work to open a new pullrequest with the two changes from you? @turboproc Couldn`t you have update your pullrequest with the two changes from @dhewg?

Since I would like to see the feature in OpenWrt, I took the feature from @turboproc and the changes @dhewg and created a new pullrequest

Anyway thank you for your work 👍

@feckert , sorry got confused on who was supposed to make the last action. Thnx for taking this one,

[dhewg]

@dhewg Is it really that much work to open a new pullrequest with the two changes from you?

If I open a PR, and need to be motivated to in-cooperate feedback, or we're possibly in the same situation as now. So... ;) Anyway, thanks for taking care of it, looking forward to seeing it finally getting merged!

[hauke]

Thank you for your patches, I applied the versions from #11088 to master in 5787e0c9fe0ae218f745ea5dcf30e20cc028842e and cc5d8ae42731b50469ab92ed00be3cfc83d4ce11.