HASwitchPlate / openHASP

HomeAutomation Switchplate based on lvgl for ESP32
https://www.openhasp.com
MIT License
695 stars 179 forks source link

Improve PC port feature support (Win32/Linux) #631

Closed kuba2k2 closed 7 months ago

kuba2k2 commented 7 months ago

This PR adds the following improvements to the "PC" build of openHASP:


It is worth noting that MQTT parameters cannot be passed in the command line arguments now - it's configured in config.json, as usual on ESP32 devices. If desired, I can try to restore that possibility, although I doubt that anyone was using it "in production" - the PC port wasn't really ready for that, due to lack of config.json support.

The PR also adds some code quality improvements:


EDIT:

I've tested all these changes on windows_sdl_64bits, windows_gdi_64bits, linux_sdl_64bits and linux_fbdev_64bits. I haven't tested on ESP32, since I don't have a working setup at hand. Even though I haven't changed nearly anything in the ESP32 implementation, testing on actual hardware would be appreciated.

kuba2k2 commented 7 months ago

Hi @fvanroie The PR should be ready for review now :slightly_smiling_face:

fvanroie commented 7 months ago

These seem like additional features, not related specifically to the PC port enhancement:

kuba2k2 commented 7 months ago

I can move them to a separate PR (PRs?). Say the word and I'll do it 😃 The "shell" feature is specific to the PC port however, because it executes operating system commands in the Linux/Windows console shell, and ESP doesn't have that.

kuba2k2 commented 7 months ago

The following commits have been reverted:

Hopefully that makes the PR a bit easier to review.

fvanroie commented 7 months ago

OK, we're testing this PR, most looks good. One issue encountered is that images on ESP32 don't seem to load anymore, which I think has to do with the path separator change.

edit: it looks like the removal of lv_fs_if_init(); is the problem.

kuba2k2 commented 7 months ago

I don't think it's possible. The change I introduced isn't even compiled on Arduino builds. It was previously set to backslash, which wouldn't even make sense on ESP32 as backslashes are almost exclusively used by Windows. Besides, my changes here are only related to jsonl loading, which doesn't change the image loading process.

kuba2k2 commented 7 months ago

Re:

edit: it looks like the removal of lv_fs_if_init(); is the problem.

That is possible. I assumed that the test code was there for the unfinished PC port, I didn't realize that it actually gets compiled on ESP32 as well. The config.json reading part shouldn't be necessary nevertheless.

fvanroie commented 7 months ago

That is possible. I assumed that the test code was there for the unfinished PC port, I didn't realize that it actually gets compiled on ESP32 as well. The config.json reading part shouldn't be necessary nevertheless.

The LVGL filesystem driver is needed on all platforms. That check is/was there for testing if the lvgl filesystem driver was working, as config.json should be there. It didn't really server any other purpose. So, the check isn't necessary but the lv_fs_if_init(); certainly is.

kuba2k2 commented 7 months ago

Corrected - that should fix the issue on ESP32.

kuba2k2 commented 7 months ago

Thanks :smiley: