tjhorner / upsy-desky

Make your standing desk smarter
https://upsy-desky.tjhorner.dev
Other
478 stars 24 forks source link

Tons of protobuf errors since updating to 1.0.1 and esphome 2023.5.1 #32

Closed jfroy closed 1 year ago

jfroy commented 1 year ago

I updated my upsy to 1.0.1 after updating to esphome 2023.5.1, and I'm now getting a ton of protobuf error messages in the logs. I did remove and re-add the integration in HASS, but I'm seeing the same logs from both the esphome add-on as well as the integration.

upsy-desky-694234 @ 192.168.1.103: Connection error occurred: Invalid protobuf message: type=16 data=b'\n\x0bdesk_height\x15\x98\x1c1a\x1a\x0bDesk Height""upsy-desky-694234sensordesk_height*\x1dmdi:human-male-height-variant2\r\xff\xff?\xb3\xe8;\x08\x80P\xd4\xfc?#8\x01': Error parsing message
upsy-desky-694234 @ 192.168.1.103: Connection error occurred: Invalid protobuf message: type=16 data=b'\n\x0bdesk_height\x15\x98\x1c1a\x1a\x0bDesk Height""upsy-desky-694234sensordesk_height*\x1dmdi:human-male-height-variant2\x18\xff\xff?\xb3F]\r\x80`\xd4\xfc?|\xf4\xfc?\xff\xff?\xb3\x85\xe1\x0e\x808\x01': Error parsing message
upsy-desky-694234 @ 192.168.1.103: Connection error occurred: Invalid protobuf message: type=49 data=b'\n\x12target_desk_height\x150V\x18\x01\x1a\x12Target Desk Height")upsy-desky-694234numbertarget_desk_height5\x9a\x99\xc9A=33KBE\xcd\xcc\xcc=Z\x05xV\xad\xbaY': Error parsing message
upsy-desky-694234 @ 192.168.1.103: Connection error occurred: Invalid protobuf message: type=16 data=b'\n\x0bdesk_height\x15\x98\x1c1a\x1a\x0bDesk Height""upsy-desky-694234sensordesk_height*\x1dmdi:human-male-height-variant2\x05\xff\xff?\xb3X8\x01': Error parsing message
upsy-desky-694234 @ 192.168.1.103: Connection error occurred: Invalid protobuf message: type=16 data=b'\n\x0bdesk_height\x15\x98\x1c1a\x1a\x0bDesk Height""upsy-desky-694234sensordesk_height*\x1dmdi:human-male-height-variant2\x18\xff\xff?\xb3F]\r\x80`\xd4\xfc?\x8c\xf3\xfc?\xff\xff?\xb3\x85\xe1\x0e\x808\x01': Error parsing message
jfroy commented 1 year ago

This is my config:

substitutions:
  name: upsy-desky-694234
  friendly_name: "Office Desk"
packages:
  tj_horner.upsy_desky: github://tjhorner/upsy-desky/firmware/base.yaml@v1.0.1
esphome:
  name: ${name}
  name_add_mac_suffix: false
api:
  encryption:
    key: <redacted>
logger:
ota:
wifi:
  ssid: !secret wifi_ssid
  password: !secret wifi_password
jfroy commented 1 year ago

Ah, looking at the device logs, it's API connection is flopping.

[09:57:03][D][api:102]: Accepted 192.168.1.9
[09:57:04][D][api.connection:959]: Home Assistant 2023.5.3 (192.168.1.9): Connected successfully
[09:57:04][W][api.connection:087]: Home Assistant 2023.5.3 (192.168.1.9): Connection closed
[09:57:04][D][api:102]: Accepted 192.168.1.9
[09:57:04][D][api.connection:959]: Home Assistant 2023.5.3 (192.168.1.9): Connected successfully
[09:57:05][W][api.connection:087]: Home Assistant 2023.5.3 (192.168.1.9): Connection closed
jfroy commented 1 year ago

Seems like this is probably a dup of #31 .

tjhorner commented 1 year ago

Thanks for the report. It does indeed seem similar to what @nreymundo is experiencing, so we'll continue discussion of that here. I will do some investigation and testing on my end and report back if I'm able to reproduce it.

tjhorner commented 1 year ago

Other than the protobuf decoding errors, does the issue manifest itself in other ways? I noticed on my install, the units for the height seem to be "?" now:

image

I also see the protobuf decoding errors in my Home Assistant logs. I'll dig deeper and see if I can determine why this is happening.

tjhorner commented 1 year ago

Ok, I found the issue. It was first introduced in this PR — when manually setting the unit_of_measurement like I am doing in the runtime-config addon it will store a reference to the const char* that you pass it but mismatch the type in get_unit_of_measurement. Consumers of the method will expect a std::string but will instead receive a pointer to a const char causing all kinds of problems, including the one we observe here.

TL;DR: It's an ESPHome issue. I will submit a PR to them so it's fixed in the next ESPHome release.

nreymundo commented 1 year ago

I noticed on my own install that the units for the height suddenly was marked as a D (It's supposed to be in CMs). Didn't really make the connection that it might be related to whatever is causing the connection issues.

image

tjhorner commented 1 year ago

I think I might need to remove the runtime "Height Units" config option for now, which is annoying but setting the units of measurement at runtime on ESPHome was never really officially supported. But in the meantime you can configure the units by adding this in the substitutions:

standing_desk_height_units: "cm"
tjhorner commented 1 year ago

I just pushed v1.0.2 which removed the runtime config option for the height units — please try it out and let me know if you still see the protobuf errors or strange height units. I tested it myself and they are both back to normal.

And it turns out my explanation earlier wasn't entirely correct: the const char* is correctly being converted to a std::string, but I suppose the const char* is getting deallocated since it leaves scope once the on_value lambda, and is invalid after that.

So it's not entirely an issue on ESPHome's side, but it will be harder to work around than I originally thought. But it should be possible.

tjhorner commented 1 year ago

Alright, I found a Very Bad And Hacky way to bring back the runtime height unit configuration option, but it works until I find something better.

Please try targeting v1.0.3-rc — if you run into any issues compiling it, please try removing everything in .esphome/external_components and .esphome/packages (I made some changes in the underlying external component that it might not refresh).

Same as before, it will require a restart before the units appear on the "Desk Height" sensor.

tjhorner commented 1 year ago

Ok, final update for the night! Sorry for bombarding you both with messages. I found a much better way to make this change which is now in v1.0.3-rc. If it works for you, please let me know and I will go ahead and fully release it.

jfroy commented 1 year ago

No worries, appreciate the rapid updates. Testing now.

jfroy commented 1 year ago

Seems to be working. I see the correct units in the device in HA, no errors in the device log or HA log.

tjhorner commented 1 year ago

Great! Thank you for confirming and for bringing attention to this issue. I'll release v1.0.3 now.

nreymundo commented 1 year ago

Seems to be working great on my side as well. Even with the encryption key that was failing before.

Thanks for solving it so quickly man!.