-
Notifications
You must be signed in to change notification settings - Fork 4.8k
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
Support Levoit Core200s (WIP, Feedback requested) #21502
Conversation
6e7ede8
to
95420a9
Compare
Hi @blenk92 |
Add berry script to support Levoit Core 200S (and possibly other too since levoit offers multiple similar devices)
Hi @barbudor, took me a bit to work myself into berry, but I think you have a point here. I re-implemented the current state in berry (the original change with the native driver is still available at https://github.com/blenk92/Tasmota/tree/core200s_native for reference) Please let me know how we can proceed to get this merged ;-) |
Great @blenk92 Merge is not under my control. |
@s-hadinger is this a good implementation of a compiled-in berry driver? I had hoped this functionality could be added by users during runtime keeping the binary small ... |
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.
Thanks, there are some changes needed, primarily remove the long delay()
(above 50ms) and do async with tasmota.set_timer()
instead.
You can also consider packaging all these in a single module, and avoid polluting the global namespace.
tasmota/berry/drivers/Core200S.be
Outdated
end | ||
return bytes("") | ||
end | ||
tasmota.delay(100) |
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.
Remove delay
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.
we are in the middle of reading a message here. Removing this delay can lead to message not being available at the point in time we are in the loop. We can maybe reduce a bit, but right now there is synchronous communication happening, which means missing a message here can lead to incorrect results of commands. Or am I missing something here?
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.
Let's stick to this, but generally speaking try to avoid this and have some sort of minimalistic state machine instead. tasmota.set_timer()
is an easy way to launch code in the future, very similar to javascript
tasmota/berry/drivers/Core200S.be
Outdated
ser.flush() | ||
ser.reset_counters() | ||
|
||
tasmota.delay(1000) |
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.
Use async function or state machine. You can't stop Tasmota for 1 full second.
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.
hmm, not sure if async behavior is easy to implement here (because the messages kind of need to be guaranteed to be send in this order, or at least first ones). If've seen weird behavior if the order of messages changes. So maybe some simple state machine needs to do the trick.
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.
See above
tasmota/berry/drivers/Core200S.be
Outdated
self.second_counter += 1 | ||
if self.second_counter == 60 | ||
self.second_counter = 0 | ||
self.set_wifi_led(tasmota.wifi() != {}) |
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.
tasmota.wifi()
is never equal to {}
. What are you trying to do ? If you want to know if wifi is connected, use tasmota.wifi('up')
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.
ah, then i guess I misread the documentation (https://tasmota.github.io/docs/Berry/#tasmota-object) . It says
Retrieves Wi-Fi connection info or empty map.
But i guess this means then that it only returns empty map if wifi is not supported (or maybe disabled) ? Maybe the documentation could be a bit more specific here...
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.
Good point. "empty map" is outdated, the object was changed later. I will update the documentation
The driver is loaded at runtime by the user. It is not solidfied. So no code size increase when not explicit loaded. Imho nice implementation when the few things Stephan reviewed are fixed. |
OK. I leave it to @s-hadinger then 👍 |
Thanks for you review. I tried to adapt according to the comments that you left. There is maybe still something open in regards of the async behavior. Maybe you want to have another look ? Just FYI: I won't have access to my Core200S for ~ 1.5 weeks. So no hurry with the re-review, because I can't test potentially requested changes on my device until then... |
* Berry coc parser keeps order of variables (arendst#21542) * Bugfix: crash when using tjpegd and LVGL (arendst#21544) * prevent crashes when using tjpegd in other places in Tasmota * do not use external tjpegd in Tasmota * Update changelogs * SML: Allow larger offset when using x to ignore bytes (arendst#21535) * Allow larger offset when using x to ignore bytes STW Klagenfurt sends 355 bytes, and the interesting values starts somewhere at position 304. Therefor we need to set a larger uint range to allow more than 255 bytes to ignore. * Update xsns_53_sml.ino --------- Co-authored-by: Theo Arends <11044339+arendst@users.noreply.github.com> * Change GPIOViewer from v1.5.3 to v1.5.4 (No functional change) * Fix GPIO16 as Transmit enable pin (arendst#21269) * Support Levoit Core200s (WIP, Feedback requested) (arendst#21502) * Support Levoit Core 200S Add berry script to support Levoit Core 200S (and possibly other too since levoit offers multiple similar devices) * Core200S: fix comments * forgotten safety check (arendst#21549) * Bump v14.1.0.1 * Berry bytes solidification (arendst#21558) * Berry prepare for bytes() solidification * Berry solidification of bytes objects * Berry solidification of `bytes` instances * Matter support for Air Quality sensors (arendst#21559) * Add default value for `SetOption151` (arendst#21560) * Update changelogs * Remove GPIO_I2S_BCLK_IN, GPIO_I2S_WS_IN * add uTouch settings * uTouch for m5core2 * Berry `input()` returns empty string and does not crash (arendst#21565) --------- Co-authored-by: s-hadinger <49731213+s-hadinger@users.noreply.github.com> Co-authored-by: Christian Baars <baars@klinikum-brandenburg.de> Co-authored-by: Theo Arends <11044339+arendst@users.noreply.github.com> Co-authored-by: Andreas Doppelhofer <andreas.doppelhofer@gmx.at> Co-authored-by: blenk92 <30472652+blenk92@users.noreply.github.com> Co-authored-by: Christian Baars <Baars@gmx.de> Co-authored-by: Jason2866 <24528715+Jason2866@users.noreply.github.com>
Description:
Hi there, I bought one of these devices https://levoit.com/collections/air-purifiers/products/core-200s-smart-true-hepa-air-purifier and figured that is uses a esp32 (the solo1 variant) to talk to another MCU via a serial protocol. I've reversed the protocol (well at least the relevant parts I would say) to control the air purifier using Tasmota.
This PR contains my initial implementation, to proof that its generally possible to that (so its still kind of in a POC state in some places). However, it should already give a rough idea on how the protocol works... However, before I spend additional efforts in making it better, I would like to request some indication if the project would generally be interested in support these devices. I say 'these' because Levoit offers a bunch of similar devices, I only own a Core200S but I could imagine that other models are similar, maybe even using an extended version of the protocol (and there are still some fields in the status messages that seem to be unused on my device).
Since this is quite a large patch already, I would also like to ask what else I would need provide in order to get the levoit devices (ore at least the core200S) officially supported.
Screenshot to show you how the web interface currently looks like:
Related issue (if applicable): fixes #
Checklist:
NOTE: The code change must pass CI tests. Your PR cannot be merged unless tests pass