-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
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 button to set date and time for thermopro TP358/TP393 #135740
Add button to set date and time for thermopro TP358/TP393 #135740
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stephan48
It seems you haven't yet signed a CLA. Please do so here.
Once you do that we will be able to review and accept this pull request.
Thanks!
Hey there @bdraco, @h3ss, mind taking a look at this pull request as it has been labeled with an integration ( Code owner commandsCode owners of
|
c5c28ca
to
36198d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All protocol level code must be in an external library.
https://developers.home-assistant.io/docs/development_checklist
https://developers.home-assistant.io/docs/api_lib_index?_highlight=pypi#basic-library-requirements
@bdraco I appologize, i started at the guideline and didn't commit what i read. Would it make sense to add this into thermopro_ble, adding a direct dependency to bleak or extend bluetooth-clocks to allow HA as a BT implementation & add this dep? do we just need a wrapper class for the time interaction codel or also a cli in the thermopro-ble repo to set the time outside of HA(like in my linked reference) for testing? |
This makes the most sense. No cli is needed. |
36198d1
to
245ebc9
Compare
I reworked the code to use the new thermopro_ble release and I started locally on a first draft for tests - still looking around try to find good stuff to learn from. Questions which are still open for me:
|
Traveling this week, so my time is limited, but I'll try to review it as soon as I can |
Good travels and no worries, take your time, I rather do it properly than cause stress around(have enough of that!) :) |
Another option to make this even simpler and easy to use would be to make it a button entity with no options and it always set the system time and am_pm based on the system settings |
245ebc9
to
70bb009
Compare
I thought about that and found this to be indeed the most elegant option. Still need to figure out testing, how to read am_pm from the HA settings and how to "link" the button to the actual device. PR itself is functional on my local test device. |
Co-authored-by: J. Nick Koston <[email protected]>
125bafb
to
a7e15d3
Compare
Thanks @stephan48 |
You are welcome. Thank you for your guidance, I appreciate this massively! |
Proposed change
This PR adds a new feature to set the device time for ThermoPro TP358/TP393 devices.
This could previously be done by opening the ThermoPro App and syncing a sensor or via this GH project.
By calling this service you can either set the system time HA is running on or a user defined time (with optional 12/24hour mode).
It was tested against a single TP358 device.
Type of change
Additional information
This is a draft, I expect to finish it with some more input over the next few days. Initial commit may be force pushed clean.
Open questions:
There is a thermopro_ble python module handling parsing of received advertisements, should the time formatting code go in there?
Checklist
ruff format homeassistant tests
)If user exposed functionality or configuration variables are added/changed:
If the code communicates with devices, web services, or third-party tools:
Updated and included derived files by running:
python3 -m script.hassfest
.requirements_all.txt
.Updated by running
python3 -m script.gen_requirements_all
.Dependency Update PR - #137381
To help with the load of incoming pull requests: