search

raetha / wyzesense2mqtt

Configurable WyzeSense to MQTT Gateway intended for use with Home Assistant or other platforms that use MQTT discovery mechanisms.
MIT License
79 stars 22 forks source link

Improved init_wyzesense_dongle log and SENSORS error handling #45

Closed AK5nowman closed 3 years ago

AK5nowman commented 3 years ago

Added some checks around SENSORS. Refactored init_sensors a bit. Added a better error message to indicate the actual error when it faults attach to the usb device.

Don't really know python all that much but this seems to work for me

AK5nowman commented 3 years ago

This addresses issue #44 so that the file now is not required and can be blank

raetha commented 3 years ago

@dale3h mind doing a quick review on this if you get a chance? I've been neck deep in way too much real life for the last month+, and haven't been able to fully dig in on any of these, but I'd like to merge this into devel at least since it looks like @AK5nowman has made some nice changes here.

AK5nowman commented 3 years ago

I’m a git novice and did not realize the commits after my PR would be included. Is standard practice to issue a PR from a dedicated branch until it’s pulled?

raetha commented 3 years ago

@AK5nowman it is standard practice to create just a temp branch for one feature, when submitting to someone else's project. However I do just a devel branch for my version updates. Don't feel bad though, the first time I submitted to another repo I made the same mistake, and ended up forking to this project because of how much I'd re-written that was contained in a single PR.

I'll take a look at the different check-ins you've done, and either pull the whole thing, or possibly just piecemeal add them directly and then close this PR.

I also apologize for the radio silence. I work in healthcare IT, and this year has been pretty crazy, but the last few months have been trying to catch up on all the stuff that was put on hold for 4 months earlier this year. Been working crazy hours and just haven't had the brain capacity to do much during the slim time I'm home.

raetha commented 3 years ago

@AK5nowman I just got done pulling the bulk of your ideas that got wrapped up in here into the devel branch. I did some of them slightly differently to better fit in with the existing code, but snagged a couple pieces outright knowing that some refactoring in the future will help them line up. Particularly your logging code, I think I want to pull some of those ideas elsewhere, but didn't have the time right this second.

The only thing I completely skipping borrowing was your sensor "alias" for the purpose of the MQTT publishing. When I set this stuff up originally, I did a fair amount of reading on HASS and MQTT. Almost everything I read said to use the unique ID for a sensor type device in the MQTT topics, and put the human readable names as values in the payload. The reasoning was that we often repurpose things like sensors and they move around. So the functional name might change. If you used that in the topic, now MQTT and/or HASS see it as a brand new device, but the old device is still out there if you used persistence. That leaves things dirty, and in the case of HASS, a bunch of dead device trails linger. Though I fully support giving folks more options, that one just feels like I'm setting them up to have a bad day.

Otherwise, please poke through what I did, and if you are up for losing the alias support, grab the current devel branch and test things out for me. In particular disabling hass_discovery, and empty sensors or config files.

AK5nowman commented 3 years ago

Yeah I tried to make it so that the alias was optional but it does make it a little messier. It will be easy enough for me to fork the devel branch and add my changes before testing

raetha commented 3 years ago

I just hate making you keep a fork. Do you have a compelling reason to avoid best practice of not using human readable names in the topic? I don't mind adding it if there is a good use case. I just haven't come up with one that out weighs the risk of duplicate MQTT topics for the same device. And at least in HA, once a device shows up, you have to actually destroy the MQTT configuration and let it rebuild in order to clean things up, or go through a lot of hoops to delete a device manually. It's a pain, have had to do it repeatedly while working on this stuff in the early days.

AK5nowman commented 3 years ago

Not sure where you are seeing these best practices, perhaps its a HA thing? Not sure how compelling it is but I've designed my system to use semantic naming as opposed to physical naming because when I consume them I don't need a cheat sheet to tell me what each MAC address is, all that info is encoded in the MQTT topic. In HA I assume you don't use the MAC address to reference a binary sensor, you would use the semantic name, yes?

When you say duplicate topic for a device, do you mean like an abandoned retained message on the old topic? When you no longer use a device that sent a retained message it really would be a good idea to delete that message from the broker anyhow, I do this anytime I rename a device.

raetha commented 3 years ago

@AK5nowman I merged the V2 branch before looking at this one again, and now there are conflicts here. As I offered in one of the issues, I'm willing to add you as a contributor and let you manage PRs directly, but unfortunately this one can't be directly merged anymore. If you still want this stuff, just let me know by either submitting a new PR against master with your V2 branch as the base, or let me know that you feel comfortable with contributor access and I'll get that set up soon.