michaelwoods / onstar2mqtt

OnStarJS wrapper for MQTT
MIT License
107 stars 40 forks source link

Refactor response handling.. and more (parses manually sent 'diagnostics' commands) #338

Open garyd9 opened 11 months ago

garyd9 commented 11 months ago

The parsing of the diagnostic command responses was moved to the command handler (in configureMQTT)

The main loop (with the timerInterval) rewritten to only send MQTT commands to request diagnostics

Changed the log level of mqtt config message to debug (they tend to fill logs with repetitive data)

If a 'diagnostics' command is sent with no options, default to requesting all supported diagnostic options for the vehicle

If the command handler doesn't understand the response data (all except location and diagnostics), publish a state topic containing the JSON returned by the server. For example, unlockDoor will publish to /homeassistant/VIN/sensor/unlockdoor/state and JSON message that indicates a success/failure status. Be aware that this is the servers response to the command, and not the vehicles response.

garyd9 commented 11 months ago

This would resolve https://github.com/michaelwoods/onstar2mqtt/issues/282

garyd9 commented 11 months ago

btw, someone who actually knows node.js should really review this. Before a couple hours ago, I never touched nodeserver, and barely dabbled in JavaScript. (I do C, C++, etc. professionally, so it wasn't too difficult to get something going.)

garyd9 commented 10 months ago

@michaelwoods Are you still actively maintaining this repo? After I did the above stuff, I've decided to spend some time this weekend to do some major work on this code. I don't know how similar it will be to the original once I go nuts with it. Do you want me to PR it back to you, or just maintain it in my own fork?

garyd9 commented 10 months ago

yeah, okay. ;) At this point, the code on my machine is VERY different. Probably just as bad for lint (so far), but it no longer needs scripts to run commands (it publishes buttons), publishes a proper MQTT device_tracker (that HA understands as a device tracker), has a diagnostics specific availability topic (for when diagnostic queries are throttled, but other commands still work) and more...

The only thing I still plan on changing is making the timerInterval (interval between polling diagnostics) an MQTT number (so a user can set it at runtime based on events. For example, when I'm home sleeping, it's fine to only poll every 2-3 hours. However, when I get up in the morning, the polling should kick back to every 30 minutes.

michaelwoods commented 10 months ago

Sorry, not sure where my comment went. Thought I had answered:

I am maintaining it on a as-needed, dependabot level to keep up with OnstarJS changes. I no longer have a command-capable OnStar subscription and mainly use it for battery level monitoring.

I like the dynamic update idea and the buttons/discovery updates are very welcome!

garyd9 commented 10 months ago

I didn't realize my continued push's to my github account were being included in the PR. ;) Right now, the code is not suitable for release. Not only is it messy, but I want to change how the diagnostic specific availability flag works so that it would be optional, not forced. ("unavailable" data won't show in a HA dashboard and many users might prefer old data to no data at all.)

As well, I need to get the configurable diagnostic polling interval code done. That keeps coming back to me having to better understand how javascript does async.

michaelwoods commented 10 months ago

gotcha, @ me when you'd like a review on it 👍

garyd9 commented 10 months ago

At the moment, this passes lint, but fails the "npm test" test. I'm not concerned about that fail simply because it doesn't seem to know about the ability to have multiple availability topics in HA autodiscovery.

garyd9 commented 10 months ago

btw, sorry I'm moving so slow on this. I'm not fluent with javascript (funny that I avoided it until now), and I managed to get sick Saturday with what I hope is "only" a bad cold. Hopefully I'll feel better by this weekend and be able to wrap it up before xmas.

michaelwoods commented 10 months ago

Take your time and take care!

BigThunderSR commented 9 months ago

Hi @garyd9, hope you are feeling better at this point! Are you planning to resume work on this PR? Thanks.

garyd9 commented 9 months ago

Hi @garyd9, hope you are feeling better at this point! Are you planning to resume work on this PR? Thanks.

@BigThunderSR

Sorry I haven't updated anything. Between being sick, the holidays, my kids being home (from college) for the holidays, etc, I haven't been as active on this as I should be. Right now, my local code has a config switch that enables/disables setting the availability of the sensors when the API isn't responding. (Sometimes stale information is better than "N/A".)

I'm fighting an issue where, quite frequently, the API is returning an error for a command, but the command works anyway. I can repeat this every morning at 7am when I have home assistant remote starting my car. It means that a "success" is always a success, but a failure may or may not be a failure. Damn annoying, but I keep trying (seemingly random) things to get a better handle on if "it MIGHT have worked" vs knowing "it absolutely didn't work."

Still to do:
1,. A command to change the timer interval for polling (and, of course, the code that actually does the work.)

  1. Figuring out how to make the automatic code checks happy with availability json that uses the newer format (which permits multiple availability flags.)