feat: support ThermoPro TempSpike (TP96x) #19
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## temp_spike #19 +/- ##
=============================================
Coverage ? 85.71%
=============================================
Files ? 2
Lines ? 63
Branches ? 8
=============================================
Hits ? 54
Misses ? 4
Partials ? 5 ☔ View full report in Codecov by Sentry. |
…are still required for the TP962R
… corresponding unit tests
|
@bdraco I think this may be close to complete, would you mind providing some feedback? |
src/thermopro_ble/parser.py
Outdated
| "TP962R": { | ||
| 0: "black", | ||
| 1: "white", | ||
| }, |
There was a problem hiding this comment.
I'd keep these as probe 0 and probe 1 since the vendor tends to add new devices with the same format and we can't be sure they will keep the color scheme in the future
There was a problem hiding this comment.
I'm actually already falling back to something like this on lines 85-92, I just use the color names when we know that the model has a specific mapping. The difference is for probe 0 I default to no affix at all.
Since we don't know if any new model has more than one probe, the alternative here would be to have "Probe 0" affixed even on single probe models. I think this is reasonable also, and would simplify the code. I started a review for that option above.
src/thermopro_ble/parser.py
Outdated
| MAX_BAT = 2880 | ||
| MIN_BAT = 1600 # ?? |
There was a problem hiding this comment.
Please make constants above and make it clear the are for the TP96 series in the name
src/thermopro_ble/parser.py
Outdated
| "TP962R": { | ||
| 0: "black", | ||
| 1: "white", | ||
| }, |
There was a problem hiding this comment.
I'm actually already falling back to something like this on lines 85-92, I just use the color names when we know that the model has a specific mapping. The difference is for probe 0 I default to no affix at all.
Since we don't know if any new model has more than one probe, the alternative here would be to have "Probe 0" affixed even on single probe models. I think this is reasonable also, and would simplify the code. I started a review for that option above.
src/thermopro_ble/parser.py
Outdated
| MAX_BAT = 2880 | ||
| MIN_BAT = 1600 # ?? |
…ing the names to clarify that they apply to TP96x series
|
@bdraco Latest commit contains all suggestions applied, how's it look? |
| if model.startswith("TP96"): | ||
| # This series can have multiple probes, so we must assume probe 1 | ||
| self.set_title(f"{name} Probe 1 {short_address(service_info.address)}") | ||
| self.set_device_name(f"{name} PROBE 1") | ||
| else: | ||
| self.set_title(f"{name} {short_address(service_info.address)}") | ||
| self.set_device_name(name) |
There was a problem hiding this comment.
The title / device name is for the whole device so we want to keep it as-is. I'll merge this PR and do a cleanup on the original PR
bdraco
changed the title
This is still a WIP, need to add support for multi-probe devices like TP962R.
Looking for feedback.
Thanks in advance!