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

Change types to match values that come back in API #422

Closed
wants to merge 1 commit into from

Conversation

Cisien
Copy link
Contributor

@Cisien Cisien commented Aug 9, 2023

This might be caused by me running Early Access builds of Unifi software/firmware, but when I have the option enabled to collect bandwidth utilization metrics in the Unifi component in HomeAssistant, I get this error:

2023-08-09 00:28:49.462 INFO (MainThread) [homeassistant.components.update] Setting up update.unifi
2023-08-09 00:28:49.744 ERROR (MainThread) [homeassistant.components.sensor] Error while setting up unifi platform for sensor
Traceback (most recent call last):
  File "/workspaces/core/homeassistant/helpers/entity_platform.py", line 362, in _async_setup_platform
    await asyncio.shield(task)
  File "/workspaces/core/homeassistant/components/unifi/sensor.py", line 205, in async_setup_entry
    controller.register_platform_add_entities(
  File "/workspaces/core/homeassistant/components/unifi/controller.py", line 230, in register_platform_add_entities
    async_load_entities(description)
  File "/workspaces/core/homeassistant/components/unifi/controller.py", line 209, in async_load_entities
    async_add_unifi_entity(list(api_handler))
  File "/workspaces/core/homeassistant/components/unifi/controller.py", line 200, in async_add_unifi_entity
    [
  File "/workspaces/core/homeassistant/components/unifi/controller.py", line 201, in <listcomp>
    unifi_platform_entity(obj_id, self, description)
  File "/workspaces/core/homeassistant/components/unifi/entity.py", line 138, in __init__
    self.async_initiate_state()
  File "/workspaces/core/homeassistant/components/unifi/entity.py", line 234, in async_initiate_state
    self.async_update_state(ItemEvent.ADDED, self._obj_id)
  File "/workspaces/core/homeassistant/components/unifi/sensor.py", line 223, in async_update_state
    if (value := description.value_fn(self.controller, obj)) != self.native_value:
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/workspaces/core/homeassistant/components/unifi/sensor.py", line 50, in async_client_rx_value_fn
    return client.rx_bytes_r / 1000000
           ^^^^^^^^^^^^^^^^^
  File "/home/vscode/.local/lib/python3.11/site-packages/aiounifi/models/client.py", line 297, in rx_bytes_r
    assert isinstance(value, int)
AssertionError

Digging in a bit further, i noticed that the values in client.raw.rx_bytes_r (and some similar fields, all of which are the bytes_r fields) were floats.

This PR addresses this by asserting that the type is a float instead of an int for these fields.

I purposefully left the test alone to ensure that data coming across the wire as ints would continue to parse as expected.

This may be a breaking change downstream, if the calling code assumes that these values are ints.

@Kane610
Copy link
Owner

Kane610 commented Aug 9, 2023

Is the data still bytes since they changed it to a float?

Asking as we're converting it to Megabytes in the integration https://github.com/home-assistant/core/blob/35718b291795c88ae324ba6a2b724430a46cfa0e/homeassistant/components/unifi/sensor.py#L47

@Cisien
Copy link
Contributor Author

Cisien commented Aug 9, 2023

After digging into this some more, i'm not entirely convinced that this is the correct change. I'm not sure what is causing the number to be displayed as a decimal type in the raw data.

Looking at the protobuf definitions I pulled from the controller, the frontend is clearly expecting this value as a 64-bit int:
/usr/share/unifi-core/app/node_modules/@ubnt/unifi-protobufs/unifi/network/types/v1/client_pb.js:

  f = message.getTxBytesPerSecond();
  if (f !== 0) {
    writer.writeUint64(
      5,
      f
    );
  }

/usr/share/unifi-core/app/service.js:

function Bq(e) {
  let t = new za.ClientStats();
  return (
    t.setAccumulativeRxBytes(e.rx_bytes),
    t.setRxBytesPerSecond(e["rx_bytes-r"]),
    t.setRxPackets(e.rx_packets),
    t.setAccumulativeTxBytes(e.tx_bytes),
    t.setTxBytesPerSecond(e["tx_bytes-r"]),
    t.setTxPackets(e.tx_packets),
    e.wifi_experience_score &&
      t.setWifiExperienceScore(e.wifi_experience_score),
    t
  );
}

additionally, these fields in the unifi UI's API calls do come back as an integer type.

@Cisien Cisien closed this Aug 9, 2023
@Kane610
Copy link
Owner

Kane610 commented Aug 9, 2023

Lets wait for the next controller to be released in final and see if others report this.

@Cisien Cisien deleted the fix-type-error-in-client branch August 9, 2023 22:13
@Cisien
Copy link
Contributor Author

Cisien commented Aug 9, 2023

Is the data still bytes since they changed it to a float?

Asking as we're converting it to Megabytes in the integration https://github.com/home-assistant/core/blob/35718b291795c88ae324ba6a2b724430a46cfa0e/homeassistant/components/unifi/sensor.py#L47

For documentation purposes: The data appears to be still in bytes per second (or at least they correlate roughly to my speedtest results), just with extra unnecessary precision.
I also confirmed that I get float numbers in the Json body when making requests with curl.

"tx_bytes-r": 1.3795298948868698E7,
"rx_bytes-r": 302065.3839301621,
"bytes-r": 1.409736433279886E7

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.

2 participants