Support for Lightpack devices / Memory leak / Race conditions

[unkie]

Summary

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Docs
• Build-related changes
• Other, please describe:

If changing the UI of web configuration, please provide the before/after screenshot:

Does this PR introduce a breaking change? (check one)

• Yes
• No

If yes, please describe the impact and migration path for existing setups:

The PR fulfills these requirements:

• When resolving a specific issue, it's referenced in the PR's body (e.g. Fixes: #xxx[,#xxx], where "xxx" is the issue number)

To avoid wasting your time, it's best to open a feature request issue first and wait for approval before working on it.

Other information:

• Adds support for one or more Lightpack HID devices.
• Fixes memory leak in JSON-RCP
• Fixes race condition by registering callback handlers before sending requests
• Fixes race condition by making an even a synchronous call

[awawa-dev]

Hi
The dbus fallback re-registers the response handler after the call returns, which reintroduces the same potential race against the async event loop plus it leaves a stale handler on the unused expected path.

Have you ever encountered a race condition in the current solution? Wouldn't it have been enough then to change QUEUE_CALL to BLOCK_CALL to prevent it (the sdbus async event loop would be blocked until the triggered handler is fully processed and a new request is made - so no race condition in the mean time)? Alternatively custom sdbus event loop which allows to pause event processing. I would prefer simpler solution here.

Regarding HID, I already have an earlier PR with a device using it, so I will probably need to extract the HID provider as a generic provider first.

QTcpSocket is still owned by QTcpServer and freed alongside it, but you are right that it should be done when the connection is closed (by changing the parent or explicitly calling deleteLater in signalClientConnectionClosedHandler)

[unkie]

Hi @awawa-dev,
Thanks for your reply.

I have encountered the race while tracking down the memory leak. Heaptrack slowed things down enough to trigger it. I believe that on slow systems the race can therefore also happen. I did not consider using BLOCK_CALL.

I saw the other PR for a hid device. I actually started already on a ProviderHid class using hidapi, and plan to migrate both the Lightpack and SyncLight driver to it.

Regarding the socket, although it will free on program exit, on my machine memory use ballooned to 400MiB+ overnight. In my case i have a HyperHDR status icon in my waybar config, which checks status every second. So i needed the fix to not have to restart HyperHDR every day.

Once i have tried BLOCK_CALL and ProviderHid is ready, i will either update this PR or close it and create a new one.

What do you think about how i implemented multiple HID devices acting as one? I could not find something in the UI to add multiple devices, so i thought this would be ok. At least it works.

Cheers!

[awawa-dev]

Hi Mark,

the PipeWire/portal part turned out to be more complicated than it initially looked, so I split it out into a separate PR: #1609.

After digging deeper into the race discussed here, I replaced the per-request uponSignal handling with a single DBus request-namespace matcher and dispatch the responses through the Qt event queue, matching them by request path. This removes the timing window where a portal response could arrive before its handler was registered, so the QUEUE/BLOCK_CALL workaround is no longer needed. Everything from matching wide namespace dbus path are immediately enqueued in Qt event queue processing and processed later when the Qt thread leaves the handler that invoked a new portal call..

Could you please have a look at #1609, test it on your setup, and add any comments or suggestions there? Once this is settled, #1607 can stay focused on the other changes.

What do you think about how i implemented multiple HID devices acting as one? I could not find something in the UI to add multiple devices, so i thought this would be ok. At least it works.

Usually HyperHDR handles one device per instance. It can also control multiple physical lights within a single instance for some devices (e.g. Philips Hue lamps), so whether multiple HID devices should be aggregated really depends on the device/API model. But those usually have dedicated JS wizards to handle the device-specific configuration (multiple devices in a single instance for Philips Hue, LIFX or Home Assistant), HID provider should be independent regardless which approach is chosen. Multiple lamps per single instance offer better performance, synchronization and use lower resources.