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

Performance for multiple attributes (brightness, colour loop & colour) for multiple lamps is a bit slow #49

Closed
RichardCroxall opened this issue Jun 26, 2020 · 17 comments

Comments

@RichardCroxall
Copy link

I am really pleased that I found this library. It has exactly the functionality I needed for my home automation system.
However when I switch on the the television, I also want to switch on 6 lamps dimmed with different colours to colour loop.
This takes a couple of seconds on my raspberry pi.

To make it go a bit faster I have written a new HueLightHelper class which inherits from HueLight.
This sends a single JSON message to each lamp with brightness, colour and colour loop in the same message.
This roughly halves the time to complete.

Rather than a single call to transfer information, multiple calls are required to initialise, set up each attribute and finally send each message. It's harder to use than the original information, but I thought the performance improvement worthwhile.

My C++ is a bit rusty as I haven't done any commercially for 20 years, so you might feel the need to polish the code, but I thing the idea is sound!

@Jojo-1000
Copy link
Collaborator

Hi, very nice that you find the library helpful.

The delay between messages is by design, because there are limitations in the ZigBee communication.
If you send too many messages too quickly, the connection is dropped by the bridge. (Some people had issues when looping through all lights and turning them all on or off without any delay)

We have something similar to the way you described already implemented on the development branch. light.transaction() allows you to send all requests in one message, so it should have less delay. It would be nice if you could check it out and give some feedback on how easy to use it is and what could be improved.

Thank you very much

@RichardCroxall
Copy link
Author

RichardCroxall commented Jun 26, 2020

I tried to paste my code, but there was an error message I didn't notice because you can only paste picture files.
Header file is:

#pragma once
#include "HueLight.h"

class HueLightHelper : HueLight
{
private:
	nlohmann::json requestJson;
	void setUpForColorXY(float x, float y);

protected:

public:
	HueLightHelper(HueLight& huelight);
	void setupForColorLoop(bool colourLoop);
	void RemoveColorLoop();
	void setupForColorRGB(int red, int green, int blue);
	void setupForBrightness(int brightness);
	void setupForOffOn(bool on);
	bool SendPutRequests(const int transition = 4);
};
CPP code file is:
#include <string>

#include "include/HueLightHelper.h"

#include "include/HueExceptionMacro.h"
#include "include/HueLight.h"
#include "include/json/json.hpp"
#include "include/Utils.h"

HueLightHelper::HueLightHelper(HueLight& hueLight) : HueLight(hueLight)
{
    refreshState();
    requestJson = nlohmann::json::object();
}

void HueLightHelper::setupForColorLoop(bool colourLoop)
{
    std::string effect = colourLoop ? "colorloop" : "none";
    if (effect != requestJson["effect"])
    {
        requestJson["effect"] = effect;
    }
}

void HueLightHelper::RemoveColorLoop()
{
    if (requestJson.count("effect") > 0)
    {
        requestJson.erase("effect");
        assert(requestJson.count("effect") == 0);
    }
}

void HueLightHelper::setupForColorRGB(int red, int green, int blue)
{
    if ((red == 0) && (green == 0) && (blue == 0))
    {
        setupForOffOn(false);
    }
    else
    {
	    const float redf = float(red) / 255;
	    const float greenf = float(green) / 255;
	    const float bluef = float(blue) / 255;

	    // gamma correction
	    const float redCorrected = (redf > 0.04045f) ? pow((redf + 0.055f) / (1.0f + 0.055f), 2.4f) : (redf / 12.92f);
	    const float greenCorrected = (greenf > 0.04045f) ? pow((greenf + 0.055f) / (1.0f + 0.055f), 2.4f) : (greenf / 12.92f);
	    const float blueCorrected = (bluef > 0.04045f) ? pow((bluef + 0.055f) / (1.0f + 0.055f), 2.4f) : (bluef / 12.92f);

	    const float X = redCorrected * 0.664511f + greenCorrected * 0.154324f + blueCorrected * 0.162028f;
	    const float Y = redCorrected * 0.283881f + greenCorrected * 0.668433f + blueCorrected * 0.047685f;
	    const float Z = redCorrected * 0.000088f + greenCorrected * 0.072310f + blueCorrected * 0.986039f;

	    const float x = X / (X + Y + Z);
	    const float y = Y / (X + Y + Z);


	    setUpForColorXY(x, y);
    }
}

void HueLightHelper::setUpForColorXY(float x, float y)
{
    requestJson["xy"][0] = x;
    requestJson["xy"][1] = y;

    //if (std::abs(state["state"]["xy"][0].get<float>() - x) > 1E-4f
    //    || std::abs(state["state"]["xy"][1].get<float>() - y) > 1E-4f
    //    || state["state"]["colormode"] != "xy")
    //{
    //    requestJson["xy"][0] = x;
    //    requestJson["xy"][1] = y;
    //}
}

void HueLightHelper::setupForBrightness(int brightness)
{
    assert(brightness >= 0);
	
    if (brightness > 254)
    {
        brightness = 254;
    }
	
    if (requestJson["bri"] != brightness)
    {
         requestJson["bri"] = brightness;
    }
}


void HueLightHelper::setupForOffOn(bool on)
{
    if (requestJson["on"] != on)
    {
        requestJson["on"] = on;
    }
}

bool HueLightHelper::SendPutRequests(const int transition)
{
    if (requestJson.count("on") == 0 &&
        requestJson.count("effect") == 0 && 
        requestJson.count("xy") == 0 &&
        requestJson.count("bri") == 0)
    {
        // Nothing needs to be changed
        return true;
    }
    else
    {
        if (transition != 4)
        {
            // ReSharper disable StringLiteralTypo
            requestJson["transitiontime"] = transition;
            // ReSharper restore StringLiteralTypo
        }

    	//if we are switching off, the do not supply a colour loop command.
        if (requestJson.count("on") > 0 &&
            requestJson["on"] == false &&
            requestJson.count("effect") > 0)
        {
            RemoveColorLoop();
        }

    	//if we are switching on and no colour loop supplier, then switch that off.
    	if (requestJson.count("on") > 0 &&
            requestJson["on"] == true &&
            requestJson.count("effect") == 0)
    	{
            setupForColorLoop(false);
    	}

        nlohmann::json reply = SendPutRequest(requestJson, "/state", CURRENT_FILE_INFO);

	    // Check whether request was successful
	    return utils::validateReplyForLight(requestJson, reply, id);
    }
}

@RichardCroxall
Copy link
Author

Sorry, although I have used git commerically (via a GUI), I am note sure with Github how to find the development branch to get your intermediate code.

@Jojo-1000
Copy link
Collaborator

Did you clone the repo via git or downloaded it as a zip file?
If you cloned it, you can use a git command line
git stash (to save your changes, because they conflict)
git checkout development

When you download a zip, you can select the branch on github and then download that branch.

@RichardCroxall
Copy link
Author

RichardCroxall commented Jun 26, 2020 via email

@Jojo-1000
Copy link
Collaborator

The only difference between < and " is that " also searches the current file path. Because the include directory is always added by cmake, there is no difference between the two when used to access the header files from the cpp files. The rest is just personal preference.

@RichardCroxall
Copy link
Author

RichardCroxall commented Jun 26, 2020 via email

@Jojo-1000
Copy link
Collaborator

When you do not use cmake, you will have to add the include directory to the include path, which you should do either way when using the library so you don't have to specify relative paths everywhere.

@RichardCroxall
Copy link
Author

Hi Jan,
Unfortunately in its current form, I was unable to use the light.transaction().
This is because (as I understand it and my C++ is a bit out of date) you have to use transaction in the form (as shown in the comments):
light.transaction().setOn(true).setBrightness(29).setColorHue(3000).setColorSaturation(128).commit();
What I really want to do is:
t = light.transaction();
If (..)
{
t.setOn(true)
}
If ( ..)
{
t.setBrightness(20)
}
t.commit();

The reason for this requirement is that I have a rules language in which you can say:
SETDEVICE Lounge.MoodLight1 OFF;
SETDEVICE Lounge.MoodLight2 OFF;
SETDEVICE Lounge.MoodLight3 ON Blue;
SETDEVICE Lounge.MoodLight4 DIM12 Red COLOURLOOP;
This gets compiled down to an automation language byte code for a virtual automation machine. Then the virtual automation machine attempts to set the lamps dynamically at runtime (when the television gets switched on). Because the runtime system may or may not be able to reach the Philips Hue hub and may or may not be able to reach the individual lamps, the runtime system maintains a wanted and an actual state for each device. Where there is a difference, it puts messages onto a queue which another thread picks up and sends messages to the hub (if it can). The second thread sends messages back to the first thread on whether it was successful.
So effectively I created a transaction by creating a JSON message. The brightness, colour, colour loop and off/on are added to the message and then the ‘commit’ part is where the message is sent and the destructor is called.
It isn’t as neat as your solution, but enables me to meet my requirement (to have pretty lights on whilst I watch television).
I hope that feedback helps you.
Regards
Richard Croxall

@Jojo-1000
Copy link
Collaborator

Thank you for the feedback. I did not think of this use case. This can be made to work, however it then requires additional care by the user.
In the current state, it is almost impossible to keep the transaction alive for longer than the light is valid.
When you enable the user to hold the transaction as a variable, the user has to take care that the light stays alive for longer than the transaction. Would you think this trade off is worth it?

@RichardCroxall
Copy link
Author

It is for my requirement. For other people, I don't know.

@RichardCroxall
Copy link
Author

Maybe I'm biased because I am thinking only about my own application but where a person wants to directly operate a lamp, I think he will be using a phone app to directly control the lamp.
Where there is some automation software which needs to control a lamp, it will be using an interface similar to HuePlusPlus. My guess is that such an application would generally be deciding dynamically what to do with each lamp rather than statically. So I think the use case I have described should be popular.
Clearly you must have other people using HuePlusPlus besides me. What do the other users need HuePlusPlus for?

@RichardCroxall
Copy link
Author

Hi Jan, you will be pleased to hear that although I am probably not using any of your new features that after a fight with the makefile to add new files and add the "src/" folder, the code compiles and switches my 6 lights off an on again with brightness, colours and color looping.
I will test whether they come on in the dark when my television is on later this evening.
regards
Richard

@RichardCroxall
Copy link
Author

The above is on the target environment of a raspberry pi rather than Windows.

@enwi
Copy link
Owner

enwi commented Jul 2, 2020

Clearly you must have other people using HuePlusPlus besides me. What do the other users need HuePlusPlus for?

Hey Richard we actually created this library for our home automation system Home++, because we did not like the other available libraries.

@RichardCroxall
Copy link
Author

Hi Jan,
I see you put an upgrade in yesterday.
I downloaded a copy and (with a few tweaks for compilation on Windows/Raspberry Pi) it works fine.
So I have deleted my "HueLightHelper" and am now using "StateTransaction".
Thanks for enhancement. I know you were sorry to lose the protection you had carefully coded in before.
So tonight we can see whether I get colour looping lights when the television is on!
regards
Richard

@Jojo-1000
Copy link
Collaborator

Very nice, I'm closing this issue then.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants