Use UnitOfDataRate

[tetienne]

Hi, thank you for your component. The number of information retrieved is great!

I have one remark related to the rate unit. Currently, it looks like MB/ps, which is weird. It should be MB/s or MBps. So I dig in the source of this repository, and I noticed that UnitOfDataRate is not used or even UnitOfInformation.

It would be great if you used these constants so this component will behave more others one.

[SaswatPadhi]

+1.

MB/ps is kinda confusing. "ps" = pico second?
As @tetienne mentioned, shouldn't it be MBps or MB/s?

[elad-bar]

released new version with fix for it, v2.0.25, pls check and let me know if works for you

thanks

[SaswatPadhi]

Thanks for looking into this.

I have "MB" as the unit (was set to MBytes previously), but I see tx/rx rates in several millions. I think the reported values are in bytes.

[elad-bar]

checking

[elad-bar]

found it and released new version with fix for it, v2.0.26

thanks

[tetienne]

@elad-bar Why copy my PR instead of merging it? I also still don’t see any usage of the units declared by HA as said in my first comment.

[elad-bar]

sorry for that, because I changed the code and done major code refactor which messed up your PR (which I saw yesterday after doing the refactor), i though I sent a message after doing the code refactoring and once noticed earlier today the the message was not sent in the PR, I manually update it according to your files and added credit as you deserve :)

again, thanks! and sorry for the mess

[tetienne]

Thx for the explanation and to have sync my PR with your refactor 👌

[SaswatPadhi]

found it and released new version with fix for it, v2.0.26

thanks! works as expected now.
just a minor note - the example data in "Call Service" section still uses "Bytes" but accepts B/KB/MB

[tetienne]

@elad-bar About my first comment, and the reason of this ticket. Can you use the official units of measurement in your integration. I gave you the linked in the same comment. Doing this, and adding a device class for each sensor will allow your integration to expose unit conversion and so you will be able to remove all the logic around this. For instance, with Synology you can see

[elad-bar]

B, KB and MB which are used are part of the official unit of measurement - the fact whether I state them manually or using the enum doesn't matter,
device class on the other hande, will represent that you are talking about (data or traffic) and you will get the functionality of changing the unit of measurement as part of the default functionality, when I developed that integration support was not available so now that it is available, I will release (in the upcoming weekend hopefully) remove the configuration and define the proper device class so that functionality will be also available for the integration.

hope it covers the need

[tetienne]

It will covers my need indeed. Thx.

About the enum or constant vs your own hardcoded values will just help to be always in sync with HA. But I agree, the UI for the end user will be the same.

FYI, I wanted to contribute to your integration to do it myself. But the arch apply here does not look like the other components I usually work on (overkiz, and toshiba for instance).

[elad-bar]

Thanks for the feature request and attempt to refactor it,
I created the component with boilerplate that assist me maintaining all my integrations with almost full separation between core home assistant code and pure logic of component,
Already made the change, need to test it before releasing new version

[elad-bar]

please check release v2.0.27

thanks

[tetienne]

@elad-bar That’s perfect! Much more user friendly and in the HA philosophy.

[elad-bar]

thanks for suggesting it and sharing that feature, was not aware of it