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.

Sign up for GitHub

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

fix Philips Hue and clean up code #2109

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

fix Philips Hue and clean up code #2109

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
273fb6a
fix Philips Hue and clean up code
Massimo53 May 15, 2024
c76f2ff
Merge branch 'daid:master' into philipsHue
Massimo53 Nov 4, 2024
bcc664a
Merge branch 'daid:master' into philipsHue
Massimo53 Feb 14, 2025
150dc2f
Merge branch 'daid:master' into philipsHue
Massimo53 Feb 18, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
70 changes: 42 additions & 28 deletions src/hardware/devices/philipsHueDevice.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,9 @@ bool PhilipsHueDevice::configure(std::unordered_map<string, string> settings)
if (username != "")
{
sp::io::http::Request http(ip_address,port);
auto response = http.get(string{ "/api/" } + username + "/lights");
string lightsListAPI = "/api/" + username + "/lights";
auto response = http.get(lightsListAPI);

if (response.status != 200) // !OK
{
LOG(WARNING) << "Failed to validate username on philips hue bridge: " << response.status;
Expand All @@ -146,7 +148,7 @@ bool PhilipsHueDevice::configure(std::unordered_map<string, string> settings)
for (const auto& entry : hue_json.items())
{
auto currentInt = string(entry.key()).toInt();
LOG(DEBUG) << "Got key from Hue API " << currentInt;
LOG(INFO) << "Got key from Hue API " << currentInt;
if (currentInt >= light_count) light_count = currentInt;
}

Expand Down Expand Up @@ -201,37 +203,49 @@ int PhilipsHueDevice::getChannelCount()

void PhilipsHueDevice::updateLoop()
{
sp::io::http::Request http(ip_address,port);

while(run_thread)
{
for(int n=0; n<light_count; n++)
{
if (lights[n].dirty)
LightInfo info = lights[n];
// Hue API can only handle about 10 requests per second
std::this_thread::sleep_for(std::chrono::milliseconds(100));
if (info.laststate != "sat-" + string(info.saturation) + "-bri-" + string(info.brightness) + "-hue-" + string(info.hue) + "-transition-" + string(info.transitiontime))
{
LightInfo info;
{
std::lock_guard<std::mutex> lock(mutex);
lights[n].dirty = false;
info = lights[n];
}
string post_data;
if (info.laststate != "sat-" + string(info.saturation) + "-bri-" + string(info.brightness) + "-hue-" + string(info.hue) + "-transition-" + string(info.transitiontime))
{
lights[n].laststate = "sat-" + string(info.saturation) + "-bri-" + string(info.brightness) + "-hue-" + string(info.hue) + "-transition-" + string(info.transitiontime);
if (info.brightness > 0)
post_data = "{\"on\":true, \"sat\":"+string(info.saturation)+", \"bri\":"+string(info.brightness)+",\"hue\":"+string(info.hue)+", \"transitiontime\": "+string(info.transitiontime)+"}";
else
post_data = "{\"on\":false, \"transitiontime\": "+string(info.transitiontime)+"}";
auto response = http.request("put", string{ "/api/" } + username + "/lights/" + string(n + 1) + "/state", post_data);
if (response.status != 200) // !OK
{
LOG(WARNING) << "Failed to set light [" << (n + 1) << "] philips hue bridge: " << response.status;
LOG(WARNING) << response.body;
}
}
}
}
std::this_thread::sleep_for(std::chrono::milliseconds(50));
lights[n].dirty = true;
}

if (lights[n].dirty)
{
sp::io::http::Request setLights(ip_address, port);
Copy link
Owner

Choose a reason for hiding this comment

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

tabs vs spaces. Please use 4 spaces instead of tabs.

Copy link
Owner

Choose a reason for hiding this comment

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

By re-creating the sp::io::http::Request for each call, it makes a new tcp connection per call. If you reuse the same object it should keep a single connection open, which is better for performance.

Did you run into some kind of bug why you needed to move this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is actually what sent me down this whole rabbit hole... it would sometimes work for the first light, but not additional lights. Wireshark packet captures showed that after the first light call, it would malform all the subsequent calls, putting the json payload before the api call. Like:

{"on":true, "sat":254, "bri":254,"hue":19660, "transitiontime": 1}put /api//lights/3/state

instead of (when Wireshark capturing Postman):
PUT /api//lights/2/state HTTP/1.1
Content-Type: application/json
User-Agent: PostmanRuntime/7.38.0
Accept: /
Postman-Token: 7780f271-cf4f-40c3-8bd4-220a074b80ba
Host: 192.168.1.104
Accept-Encoding: gzip, deflate, br
Connection: keep-alive
Content-Length: 45

{"on": true, "bri": 254, "transitiontime": 1}

So I pulled it out to make a separate connection each time and that did the trick, no more malformed API packets after that. I'm not sure why that is the case, I was on a bit of a time crunch when I was working on it (had a game scheduled) so I was certainly in "just make it work" mode.

I don't know how many people even use the Hue feature, I just happen to already have them in my game space so might as well. I'll tinker with and clean it up some more but I can't promise any kind of timeline for it, so feel free to reject for now if you'd like and I can open a new PR if I make it any better.

string APIEndpoint = "/api/" + username + "/lights/" + string(n+1) + "/state";
LightInfo currentLight = lights[n];
string postData;
if (currentLight.brightness > 0) //light is on
{
postData = "{\"on\":true, \"sat\":" + string(currentLight.saturation) + ", \"bri\":" + string(currentLight.brightness) + ", \"hue\":" + string(currentLight.hue) + ", \"transitiontime\":" + string(currentLight.transitiontime) + "}";
}
else
{
postData = "{\"on\":false}";
}

// call the API
auto resp = setLights.request("PUT", APIEndpoint, postData);
if (resp.status == 200) // OK
{
lights[n].dirty = false;
Copy link
Owner

Choose a reason for hiding this comment

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

By moving this here, and removing the lock, you created a race condition, which is likely why you needed the 2nd dirty check construct.

}
else
{
// Do nothing but keep the light flagged for future try
}
}
else
{
// Do nothing, the light is correct already
}
}
}
}
Loading