Refactor response handling.. and more (parses manually sent 'diagnostics' commands) #338
Conversation
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.
|
This would resolve #282 |
|
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.) |
|
@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? |
Vehicle location now publishes a HA autodiscovery topic. No more need for manual YAML in configuration.yaml.
The code was designed to only send each diagnostic autodiscovery topic once, but a bug was causing it to send the topics/payloads for autodiscovery for each sensor every single time diagnostics were parsed.
Because onstar tends to throttle diagnostic requests, added a diagnostic specific availability flag. This is in addition to the overall availability flag. Now, when diagnostics get throttled, they'll get marked unavailable while leaving command functions (and location data) available.
|
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. |
Most of the commands now publish an MQTT button discovery topic (allowing them to be
used without scripting.)
Location now publishes a proper MQTT device_tracker for discovery that works without
any oddness.
When a command succeeds (other than location/diagnostics) or fails (all), it will
publish an MQTT Event (but MQTT events are broken in HA) giving lots of information
about the success or failure. While the events are broken, it can still be used
as a state trigger. (Trigger on the event's 'Timestamp' attribute changing. A simple
action for this is to send a notification of '{{ trigger.to_state.attributes.friendlyMessage }}'
Probably some more things changed I forgot about. I'm not done yet.
|
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! |
src/index.js
Outdated
| button_discovery.push( | ||
| client.publish(etopic, JSON.stringify(epayload), {retain: true})); | ||
|
|
||
| Promise.all(button_discovery); |
There was a problem hiding this comment.
this Promise.all isn't doing anything, not being returned nor awaited
There was a problem hiding this comment.
I'm not at all familiar with javascript async code and just followed the model that was used elsewhere. ;) I was under the impression (only from context) that "Promise.all(list)" would ensure everything in the "list" was completed. Would it be better to just call client.publish() multiple times without using promises?
There was a problem hiding this comment.
The all() does that, but nothing is using the one promise to represent the many. If a publish were to fail (without a .catch()), it may throw an unhandled rejection exception and crash. Not really the biggest deal here, docker will restart the service and these actions are mostly fire and forget.
Removing the promise collection and .all() would be fine.
src/index.js
Outdated
| } | ||
| } | ||
| logger.debug(`Status: ${data.status}, response: ` + JSON.stringify(data.response.data)); | ||
| const location = _.get(data, 'response.data.commandResponse.body.location'); |
There was a problem hiding this comment.
can move this within the getLocation case
There was a problem hiding this comment.
lol. It's outside the case because lint was complaining about it being inside the case (unless enclose the case in {} )
src/index.js
Outdated
| client.publish(topic, JSON.stringify(state), {retain: true}) | ||
| ); | ||
| } | ||
| Promise.all(publishes); |
There was a problem hiding this comment.
same here, Promise.all batches multiple into one but you still need to handle the one promise
src/mqtt.js
Outdated
| { | ||
| payload_available : 'true', | ||
| payload_not_available : 'false', | ||
| topic : this.getDiagnosticAvailabilityTopic() |
There was a problem hiding this comment.
Would this end up marking the device as unavailable? Thought that wasn't what you wanted by adding this topic.
There was a problem hiding this comment.
The end result is that there will be two availability topics for the diagnostics. One is marking the availability of the entire onstar2mqtt program, and then there's an additional one that swaps to unavailable when a diagnostics command returns an error. Both have to be true/online in order for the associated entities to be flagged available. This allows diagnostic sensors to be marked unavailable while the rest of the commands are still available.
src/mqtt.js
Outdated
| let unique_id = `${this.vehicle.vin}-${_name}` | ||
| unique_id = unique_id.replace(/\s+/g, '-').toLowerCase(); |
There was a problem hiding this comment.
Pull this out as it's repeated several times.
|
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. |
|
gotcha, @ me when you'd like a review on it 👍 |
Address several concerns from @michaelwoods, including removing Promises from synchronous code. (The client.publish() stuff appears to work just fine.) Re-refactor some of the config topic payload code to remove duplicated code.. Note that I'm away that there's very little (if any) error checking when something doesn't publish to MQTT. There really isn't much I can do, either.
|
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. |
|
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. |
|
Take your time and take care! |
|
Hi @garyd9, hope you are feeling better at this point! Are you planning to resume work on this PR? Thanks. |
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: |
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.