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

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

Merged
merged 36 commits into from
Feb 22, 2024

Conversation

kuba2k2
Copy link
Contributor

@kuba2k2 kuba2k2 commented Feb 8, 2024

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

  • building without MQTT (fix)
  • storage support (OS filesystem) + printing config directory path
  • config.json support
  • Win32: GDI port (without SDL2)
  • single void setup()/void loop() entrypoints for PC and Arduino builds
  • Linux/Win32: threaded LVGL task
  • Linux: fbdev port
  • Linux: mouse and touchscreen support
  • Linux: support LCD backlight control
  • Linux: specify fbdev and evdev paths in config.json
  • set max framerate in config.json
  • Linux/Win32: command console support (stdin)
  • Linux/Win32: execute shell commands, process output
  • support "action" tag for all controls
  • replace %xyz% values in commands specified in "action" tag (optional, build-time-disabled by default)
  • command scheduling
  • Linux/Win32: MQTT on a separate thread (Paho async)
  • submit lv_drivers PR: Update PC port features fvanroie/lv_drivers#1

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:

  • HASP_TARGET_ARDUINO, HASP_TARGET_PC macros instead of defined(WINDOWS) || defined(POSIX)
  • logging output alignment fixes, unifying sources of _chip_model, _core_version
  • fixed memory corruption (out-of-bounds read) caused by incorrect usage of memcpy()
  • used implicit (auto) type instead of useconds_t to fix building on Alpine Linux (musl libc)
  • clang-formatted code here and there

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.

Copy link
Collaborator

@fvanroie fvanroie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, but this PR is huge... I really do appreciate the PC port and new Linux/Windows drivers.

There also seems to be additions which are unrelated to the PC port... that makes this harder to evaluate (shell scripts, schedule commands, publish events??)

I hope this doesn't break anything and doesn't cause issues on the ESP32 side...
It would have better to split this over multiple, smaller PRs... I'll see what I can do to review this though.

@fvanroie
Copy link
Collaborator

fvanroie commented Feb 11, 2024

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

  • Linux/Win32: execute shell commands, process output
  • support "action" tag for all controls
  • replace %xyz% values in commands specified in "action" tag (optional, build-time-disabled by default)
  • command scheduling

@kuba2k2
Copy link
Contributor Author

kuba2k2 commented Feb 11, 2024

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
Copy link
Contributor Author

kuba2k2 commented Feb 12, 2024

The following commits have been reverted:

  • Allow changing LVGL refresh and animation period
  • Support event data substitution in action commands
  • Implement command scheduling
  • Allow publishing MQTT message even with action tag

Hopefully that makes the PR a bit easier to review.

Copy link
Collaborator

@fvanroie fvanroie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix disk access path separator on Linux

I think this breaks the path on Arduino

@fvanroie
Copy link
Collaborator

fvanroie commented Feb 14, 2024

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
Copy link
Contributor Author

kuba2k2 commented Feb 14, 2024

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
Copy link
Contributor Author

kuba2k2 commented Feb 14, 2024

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
Copy link
Collaborator

fvanroie commented Feb 15, 2024

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
Copy link
Contributor Author

kuba2k2 commented Feb 15, 2024

Corrected - that should fix the issue on ESP32.

@fvanroie fvanroie merged commit 837061b into HASwitchPlate:master Feb 22, 2024
@kuba2k2
Copy link
Contributor Author

kuba2k2 commented Feb 22, 2024

Thanks 😃

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