WLED icon indicating copy to clipboard operation
WLED copied to clipboard

Emulating Alexa Devices on Segments

Open geforcefan opened this issue 1 year ago • 12 comments

The feature is intended to behave as follows:

  • In the Sync setup, users can activate segment Alexa device emulation.
  • At least two segments are required to be emulated.
  • Turning on and changing brightness on a segment will turn on the main strip to full brightness.
  • Turning off a segment will not turn off the main strip (we might consider checking whether all devices are also turned off to turn off the main strip).
  • Color changes on a segment will have the same behavior as on the main strip.

Technical Refactorings

  • Code is structured into modular functions, promoting better separation of concerns and making it easier to understand and extend.
  • Initialization of segments and presets is separated, enhancing code clarity and maintainability.
  • Lambda functions are utilized to react to changes for different device types, including the strip, segment, and preset.
  • Operations such as turning on/off, brightness adjustment, color setting, and segment control are extracted into individual functions for better maintainability.
  • Switch-case statements are used for handling different property changes from Alexa devices, improving code clarity and reducing nested if-else statements.

geforcefan avatar Mar 13 '24 20:03 geforcefan

Hi, can you explain a bit more about the logic of your software design? Especially I'm curious to know how you handle the fact that segments are very volatile in nature.

Basicially each click in the segments or presets UI panels can lead to segments being deleted, reordered, or re-allocated - also many interactions with the JSON API will cause deletion of all existing segments.

1/ This implies that any change in presets or segments will lead to deletion and re-announcement of alexa devices?

2/ Also it looks like you keep a Segment pointer with each segment-specific alexa instance... I think that when you later try to modify a segment using the pointer, this could cause access to unallocated memory (use-after-free). Because its basicially impossible to verify that an old pointer (copied when the segment did exist) is still a valid segment pointer when an alexa command arrives.

softhack007 avatar Mar 14 '24 18:03 softhack007

@softhack007

  1. That's why I wanted to have an open discussion about the logic. I can describe the motivation behind this idea in my home scenario. I have a Dartsboard installed which has analog CT LED strips and a dartswall with RGB LEDs outside. Instead of installing two LED drivers, I decided to use one D1, because 4 outputs are enough for them, but I wanted to separate devices with the 'Make a segment for each output' approach. So when someone decides to use their segments as separate Alexa devices, the user should be aware not to change the count of segments that often. In my current version (not committed), I am calling initAlexa after the interaction with the JSON API. So the real-world usage would be, all segments are available via Alexa. When a segment is not available, the device is unresponsive from the perspective of Alexa. However, when a segment becomes available again, the device will respond again. If they are not available in the meantime, you need to adjust your Alexa devices setup.

Maybe there is a better way to emulate different devices, but I don't see anything close to segments for full control. I know segments are not that suitable for this, but it's the closest thing I can use for separating devices. WLED's architecture is not easily changeable to get a better fit, so this was my 'uncomplicated' solution.

  1. I guess you are talking about the callback line for segments:
espalexa.addDevice(new EspalexaDevice(name.c_str(), [i](EspalexaDevice* dev) { 
      Segment &segment = strip.getSegment(i);
      onSegmentChange(dev, &segment); 
    }, EspalexaDeviceType::extendedcolor))

I am passing the index to the lambda function and fetch the segment there. When the index is out of bounds, strip.getSegment() will return the main segment, which is not the right way I guess but won't result in accessing a segment which is deleted. A simple index >= _segments.size() before calling the onSegmentChange and getSegment functions should be enough.

geforcefan avatar Mar 14 '24 20:03 geforcefan

Another topic - please make sure that your changes will compile on esp32, esp8266 and -S3/-S2/-C3.

There is a build error currently on esp32:

wled00/alexa.cpp: In function 'void initAlexaForSegments()': wled00/alexa.cpp:169:50: error: 'to_string' is not a member of 'std' std::string name = std::string("Segment ") + std::to_string(i); ^ *** [.pio/build/esp32dev/src/alexa.cpp.o] Error 1

Edit: the solution could be to only use the arduino String class. As a quick win, "String" is also compatible with "FlashStringHelper".

softhack007 avatar Mar 15 '24 10:03 softhack007

@softhack007 I've made some adjustments and added additional sanity checks. If you have a moment, I'd appreciate it if you could review my PR. It should be nearly ready unless there are any suggestions regarding the behavior I've described above. ~I also need to add a checkbox for emulating segment Alexa devices in the user interface.~ Thank you!

The feature is intended to behave as follows:

  • In the Sync setup, users can activate segment Alexa device emulation.
  • At least two segments are required to be emulated.
  • Turning on and changing brightness on a segment will turn on the main strip to full brightness.
  • Turning off a segment will not turn off the main strip (we might consider checking whether all devices are also turned off to turn off the main strip).
  • Color changes on a segment will have the same behavior as on the main strip.

geforcefan avatar Mar 16 '24 17:03 geforcefan

@softhack007 I've made some adjustments and added additional sanity checks. If you have a moment, I'd appreciate it if you could review my PR. It should be nearly ready

@blazoncek may I pass this review on to you? I think you're the best one to double-check that segment and strip are used in the safe / recommended way.

softhack007 avatar Mar 18 '24 23:03 softhack007

@geforcefan I think the failed CI builds are not "your responsibility" - looks like something is broken with the platformIO repository in general today.

Tool Manager: Installing platformio/tool-scons @ ~4.40400.0 

Error: Could not find the package with 'platformio/tool-scons @ ~4.40400.0' requirements for your system 'linux_x86_64'

softhack007 avatar Mar 18 '24 23:03 softhack007

First off, re-base this for 0_15 branch and squash (follow the Wiki). Then I can have a look.

blazoncek avatar Mar 19 '24 05:03 blazoncek

@blazoncek @softhack007 I apologize for responding late. I was unwell this week due to a severe infection, but I am now feeling better. I have a question for you all: How does Alexa determine the initial values (such as color and brightness) of the device?

geforcefan avatar Mar 22 '24 18:03 geforcefan

Unfortunately I do not own Alexa and have no knowledge of its workings.

blazoncek avatar Mar 22 '24 19:03 blazoncek

@geforcefan are you still working towards this PR to be accepted?

MadTooler avatar Jun 15 '24 13:06 MadTooler

@MadTooler Unfortunately, there wasn't much response or interest in this project. Additionally, none of the project's authors were available to review the logic I adjusted during my refactoring. As a result, I haven't made any further changes since then. While it is functional, I still need to pull the latest changes from the main branch to ensure it works correctly. However, I believe there isn't significant interest in this feature

geforcefan avatar Jun 18 '24 07:06 geforcefan

@geforcefan Thanks for the reply. it is too bad you were not able to get others on board. If I was working from scratch, I would not be involving alexa devices, but I am adding lighting and features to a family member's home that already has them in place.

I will try plugging in your files and see what happens.

Thanks again.

MadTooler avatar Jun 30 '24 05:06 MadTooler