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

Emulating Alexa Devices on Segments #3822

Closed

Conversation

geforcefan
Copy link

@geforcefan geforcefan commented Mar 13, 2024

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.

@softhack007
Copy link
Collaborator

softhack007 commented Mar 14, 2024

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.

@geforcefan
Copy link
Author

@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.

@softhack007
Copy link
Collaborator

softhack007 commented Mar 15, 2024

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".

…d stuff, little sanity check on segments whether the segment index is existing on alexa change callbacks
@geforcefan
Copy link
Author

geforcefan commented Mar 16, 2024

@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 geforcefan changed the title Emulating Alexa Devices on Segments [WIP] Emulating Alexa Devices on Segments Mar 16, 2024
@geforcefan geforcefan marked this pull request as ready for review March 16, 2024 18:16
@softhack007
Copy link
Collaborator

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

softhack007 commented Mar 18, 2024

@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'

@blazoncek
Copy link
Collaborator

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

@geforcefan
Copy link
Author

@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?

@blazoncek
Copy link
Collaborator

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

@MadTooler
Copy link

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

@geforcefan
Copy link
Author

@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

@MadTooler
Copy link

@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.

@softhack007 softhack007 added the rebase needed This PR needs to be re-based to the current development branch label Aug 15, 2024
@geforcefan geforcefan closed this by deleting the head repository Oct 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rebase needed This PR needs to be re-based to the current development branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants