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

Set "message" mode on startup #10

Merged
merged 4 commits into from
Dec 28, 2022
Merged

Set "message" mode on startup #10

merged 4 commits into from
Dec 28, 2022

Conversation

mag1024
Copy link
Contributor

@mag1024 mag1024 commented Oct 5, 2022

UPStart can sometimes leave the device in the "pulse" mode, which leads to the library not being able to interpret any of the PIM responses.
To support the write register command, I'm replacing the boolean raw parameter of send() with a command enum. The equivalent of "raw=True" can be achieved by setting the command to None, but it can also be overridden from the default TX_UPB_MSG to other valid commands, like WRITE_PIM_REGISTERS.
This PR is stacked on top of #9.

@mag1024
Copy link
Contributor Author

mag1024 commented Dec 17, 2022

Friendly ping :)

@gwww
Copy link
Owner

gwww commented Dec 23, 2022

Friendly reminder ;)

#8 (comment)

@mag1024
Copy link
Contributor Author

mag1024 commented Dec 24, 2022

Sorry, I'm not sure I follow.

Pick one improvement and make that provide that fix as a PR.

That is precisely what #9 and #10 are. Small improvements, that are unrelated to the feature discussed in #8.

Or are you referring to the fact that #10 is an apparent super-set of #9?
That's because for branching reasons it was more convenient to stack the changes. If you review/merge #9 first, #10 will become much smaller.

@gwww
Copy link
Owner

gwww commented Dec 24, 2022

I’ll take a look in a couple of days and give concrete suggestions on what I’m looking for.

@gwww
Copy link
Owner

gwww commented Dec 27, 2022

Yes, it is because the two PRs are "stacked" - I did not realize that was what was happening. I've merged #9. It is possible that you can rework this one so that only the changes for message mode are part of the PR. It will be easier to review and test that way.

Once this is merged I'll issue a new release of the lib. If you want to make the changes to Home Assistant to use the new changes I will leave those to you. I will end up reviewing them as the maintainer of that integration.

I will also do a bit of housekeeping when issuing the new lib. Some of the libs in pypoetry.toml need updated minimum version numbers.

@mag1024
Copy link
Contributor Author

mag1024 commented Dec 28, 2022

Thanks! Synced the branch to pick up the merged tool changes, so the diff now includes only the message mode-related changes. PTAL.

@gwww
Copy link
Owner

gwww commented Dec 28, 2022

Just tested it. It works. I noticed that the 70028E mode set is sent twice. I don't have time to look at that right now but would like to understand why. It's not the worst if it remains that way, but would like to understand.

@gwww
Copy link
Owner

gwww commented Dec 28, 2022

Doh. Nevermind on the double write. It's a queued write, then it's written. Let me play with this a bit more before merging.

@gwww gwww merged commit 5b50702 into gwww:main Dec 28, 2022
@gwww
Copy link
Owner

gwww commented Dec 28, 2022

Thanks!

@gwww
Copy link
Owner

gwww commented Dec 28, 2022

0.5.2 is published on PyPi.

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