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

Add independent sensors and remove attributes from device_tracker #30

Merged
merged 14 commits into from
Dec 2, 2024

Conversation

Chaoscontrol
Copy link
Contributor

They show unavailable after HA restart, and reappear after receiving data once again.

@wtadler
Copy link
Collaborator

wtadler commented Nov 30, 2024

Thanks so much for the PR! Two requests:

  • This code probably works (based on my testing of what you shared before) but is repetitious. Can you find a way to write this without repeated code? I was poking around at other core integrations to see how they do it. I'm not sure which core integrations are the best ones to draw inspiration from, but most do something like this to make sure no code is repeated. Do you want to try giving something like that a shot? I know you used the device_tracker.py from this repository as a template with Claude but I'm not sure it's the ideal model; it's probably fairly out of date in terms of Home Assistant standards. There are probably good ways to prompt Claude to do all this!
  • Can you also include all of the other sensors that LeafSpy exposes (except the ones that go into the device_tracker)? Taking care of the above should make this a cinch!

@Chaoscontrol
Copy link
Contributor Author

Chaoscontrol commented Nov 30, 2024

Oof. Ok, I can try.

Re the other sensors, which ones you mean? The attributes within the device_tracker are all that are exposed AFAIK. I know because I tried creating Quick Charge count sensors, till I found the docs saying those weren't sent to server (even if logged by Leafspy in its txt).

I didn't see many others that were too useful. I can still try, but please clarify which ones.

Here they are (page 71):


1. • user=xxx User ID
2. • pass=xxx Password
3. • DevBat=xx Phone Battery Level %
4. • Gids=xxx
5. • Lat=xxx.xxxxx
6. • Long=xxx.xxxx
7. • Elv=xxx Elevation in meters
8. • Seq=xx Sequence number of transfer
9. • Trip=xx Trip number
10. • odo=xxx Odometer in km
11. • SOC=xx.xx State of Charge
12. • AHr=xx.xxxx Current AHr capacity
13. • BatTemp=xx.x Average Battery Temperature
14. • Amb=xx.x Ambient Temperature
15. • Wpr=xx Front Wiper Status (FB)
16. • PlugState=x Plug State (EL)
17. • ChrgMode=x Charge Mode (EM)
18. • ChrgPwr=xxxx Charge Power in watts
19. • VIN=xxxxxxxxxxxxxxxx
20. • PwrSw=x Power Switch state 0=off 1=on
21. • Tunits=x Temperature Units (C/F)
22. • RPM=x Motor RPM
23. • SOH=xx.xx State of Health
24. • Hx=xx.xx Hx
25. • Speed=xx.x Speed m/sec
26. • BatVolts=xxx.xx Battery Voltage
27. • BatAmps=x.xx Battery Amps

@wtadler
Copy link
Collaborator

wtadler commented Nov 30, 2024

Sorry, my bad—I forgot that I had been using device_tracker.see to spin out latitude/longitude into a separate device_tracker.

I would include all of those except for user, pass, lat, and long. (Almost all of these are already included as device_tracker attributes. Ideally, with this PR, we'll also remove all non-location-related attributes from the device_tracker, and I wouldn't want to decrease the overall functionality provided by the integration.) This would be in keeping with the general approach by Home Assistant components to expose lots of entities and leave it to the user to ignore or disable the ones they don't think are useful. I could imagine a user doing something creative or useful with many of these!

@Chaoscontrol Chaoscontrol changed the title Add 3 sensors for SOH, SOC, and Odometer Add independent sensors and remove attributes from device_tracker Nov 30, 2024
@Chaoscontrol
Copy link
Contributor Author

Ok, it's ready. I have only been able to test using the test packets from Leaf Spy and a single car trip, but apparently everything is working fine.

@wtadler
Copy link
Collaborator

wtadler commented Dec 1, 2024

Great work! Works well for me. Thanks for the legwork here. (I mean that literally, given that testing it requires getting in your car.)

A few outstanding things that I want to address before merging. Feel free to take a crack at them, or I can get to them eventually.

  1. Whether to interpret temperatures as C or F (e.g., here) should be determined by the "Temperature unit" that LeafSpy reports. (I noticed this because my temperatures being reported were way too high because of units being incorrect.) And then we don't need temperature unit to be a dedicated sensor.
  2. "Power switch" should be a binary_sensor with device_class running
  3. I turned on the wiper but the status remained "unknown," not sure why. Might have to do with the decoding dictionary.
  4. Do you know what the Leaf "Sequence number of transfer" is? It looks like just an integer that increments by 1 periodically. The LeafSpy documentation isn't much help. We might not need this one.
  5. It would definitely good to address the problem where the entities don't restore after a restart, so that people could depend on these entities to show their current battery status, for example. I took a crack at this but couldn't make it work. I see you already posted about this in the HA dev-support Discord channel. Hopefully someone chimes in! (Eventually it might also be good to see if anyone there wants to look over this integration in general to make sure we're doing things basically right.)

@Chaoscontrol
Copy link
Contributor Author

Damn, you're demanding! 😂 No need to perfect this in a single PR! Anyway, here I go again.

  1. Done. Obviously this doesn't work on the go. If you change your units on LeafSpy, you need to restart HA or it will show wrong. But after restart, the sensor gets initialised with the right units.
  2. Done.
  3. I noticed this too. Double checked and it's not a sensor problem. My Leaf Spy seems to always send the message on 'Wpr': '0' despite of what I do with the wipers, which means that prob some Leafs are not reporting this to LeafSpy at all. That's why it defaults to unknown. No idea which version of Leaf would work.
  4. It seems to be the sequence of messages from the app. It increases with each sent message. I wouldn't delete it, as it could be useful for data continuity or knowing if you've missed a message. It's done, and people can always disable it.
  5. This is the motherload. In theory is actually done in the code, but it obv doesn't work. I might try again at some point, but it seems to be more complex than it seems. I don't think it's too bad atm tho. I would publish the current version and then see if we can add that new feat for more convenience.

@wtadler
Copy link
Collaborator

wtadler commented Dec 1, 2024

Great! I'll test this later today, but it looks good. Thanks for your thoughts re: the wiper sensor and the sequence number.

The reason I want to get all these fixes done is that, with the removal of the attributes, this will be a breaking change for the (tens of?) integration users. So I am hesitant to merge into the main branch or make a release until it's totally ready! But if you want to try to do the restoration work in a second PR, I get that. Once I can test this and confirm that the new changes work, I'll merge this PR into a sensor_entities branch that I just made. And a second PR on restoration could be merged into that branch and then back to the main branch eventually.

(By the way—and maybe this is a conversation to save for that second PR—in looking through the branches just now, I see that the original integration author @jesserockz had been working in a separate branch on sensor entities, but didn't finish the work. Someone reported "hitting several issues immediately" with those changes, so your PR is probably farther along. I haven't tested his code or even noticed it until just now. But you could perhaps look to the code in that branch to see if the restoration functionality works.)

@Chaoscontrol
Copy link
Contributor Author

Thanks!

No worries, I understand and you're right. Tbh I don't know what's good practice in dev in Github, so happy to go along with your call.

I will have a look at that other PR, but tbh looking at other code is not super helpful to me personally, as I cannot reproduce or see what is it exactly that makes it work. But yeah I can provide it to the AI and ask to use the key points. Will work on that when I can during the next few days and report on progress.

@wtadler
Copy link
Collaborator

wtadler commented Dec 1, 2024

Awesome, thanks! Yeah, how to handle partially-complete code is a judgment call. This is a relatively sleepy HACS integration, so people are probably comfortable with some measure of experimentation, but I'd like to have stable sensors before I push the release out to users (with a big "breaking change" warning).

For the sensors, you could also try testing out the branch and seeing if the restoration works!

@Chaoscontrol Chaoscontrol deleted the patch-1 branch December 2, 2024 12:48
@Chaoscontrol Chaoscontrol restored the patch-1 branch December 2, 2024 12:48
@Chaoscontrol Chaoscontrol reopened this Dec 2, 2024
@wtadler wtadler changed the base branch from main to sensor_entities December 2, 2024 13:19
@wtadler wtadler merged commit 12b1bb3 into jesserockz:sensor_entities Dec 2, 2024
2 checks passed
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