sidoh / esp8266_milight_hub

Replacement for a Milight/LimitlessLED hub hosted on an ESP8266
MIT License
936 stars 220 forks source link

CCT brightness/color_temp state, determine group ID from command for on/off #265

Closed jstans closed 6 years ago

jstans commented 6 years ago

Wish I had a better title but I'm really not sure what's happening here.

So, I've just set up my ESP8266 with compiled source from the master branch and it is now working with my CCT bulbs (and it's great!) but I'm having a strange issue that I'm now trying to diagnose. I am using Home Assistant with MQTT and the esp8266 seems to get in this state where it seems to stop sending any commands and seems to get confused with the state it sends out over MQTT. This is fixed temporarily with a restart of the esp8266 until it happens again.

A reliable way of entering the state seems to be to try and set a warm colour on home assistant though I am confident there are other ways. Currently assuming this is sending an out of range value or something which triggers the bug. After this no commands work and weirdly when using a remote to send commands it sniffs the packet correctly but sends the MQTT update for the wrong group.

I should also note that despite the group_state_fields the only fields I see in the MQTT messages are state, bulb_mode and color.

sidoh commented 6 years ago

I think this is probably because CCT brightness/temp commands involve 10x or more packets than are usually required. This is basically because it has to be decomposed into a series of up/down commands.

From my experience, when espMH tries to send too many packets at once, the TCP stack gets overwhelmed and it gets into a funky state (sounds like this is what you're describing).

I'd recommend running through the suggestions here:

https://github.com/sidoh/esp8266_milight_hub/wiki/Performance-Tuning

In particular for your situation:

jstans commented 6 years ago

Thanks for the response. Hopefully I can find some resolution to this as the colour/brightness control was the main reasoning for this (specifically flux transitioning).

The brightness/temp commands requiring so many more packets, is this 100% necessary or is it doing this for some transition effect? I have noticed that when you set a warm colour even from another warm colour it transitions to pure white first (I don't remember seeing this behaviour with the original milight app?) before changing to a warm colour. If you're dragging a slider it will then generally repeat this cold to warm switch, taking a few seconds to normalise.

The response definitely appears to get slower when adjusting the brightness up and down rapidly, but once it clears the queue it does keep on working. It seems to only be when I set the colour excessively high on the warm scale that it shuts down instantly and experiences this odd behaviour. Are more packets sent if the colour is warmer? In which case I imagine it would be overflowing a buffer limit for that individual command rather than overflowing the command queue. Perhaps a buffer limit could be added somewhere to prevent this happening if it can be tracked down?

In regards to the hardware handling the requests. The gateways appear to be fine doing this, so I'd hate to be moving backwards. Presumably using a LT8900 and eliminating the emulation would go a long way to improving this? Otherwise I may have to look at the gateway option. I know this is out of the scope of this project, but theoretically wouldn't an RF radio connected to a raspberry pi have significantly more cpu power to keep up?

UPDATE:

So while writing this post I did some more tests and updated the code to 1.7.0-dev7 and set the values state_flush_interval from 10000 to 20000, cleared mqtt_update_topic_pattern and set packet_repeat_minimum from 3 to 2. I then put the warmness to 100% and also transitioned back and forth and saw that it was handling it slowly but fine (just like it was before, only with the higher values). I then set the values back to the values they were at previously and it still seems to be working just fine. Suggesting some recent change may have alleviated/improved it? I am still seeing the somewhat slow and "cold-first" temperature switching though.

I'm also still only seeing the 3 state fields in the MQTT packet despite the group_state_fields setting.

sidoh commented 6 years ago

Yes, it's done this way due to the way the CCT protocol is implemented. There isn't a "set brightness to X" command, so this is the only way to achieve setting brightness directly.

Ah, I neglected to ask if you were on 1.7. #170 fixed a bug with exactly this functionality.

Keep in mind that repeat throttling is disabled by default. Set throttle sensitivity to something >0 to enable it.

MQTT state only includes fields with known values.

jstans commented 6 years ago

So I've just connected up the old milight gateway to test this.

For brightness I can definitely understand this, especially since the milight app itself only has a dial for controlling the brightness. But the issue here is with the colour temperature. You can most definitely adjust the temperature using the gateway and app and it responds instantly and won't go to a cold white first. In fact sniffing clearly shows a single packet being sent. But adjusting via the web gui or home assistant via mqtt using the esp8266 definitely seems to do a transition regardless of whether a brightness adjustment was involved. I'm quite keen to find a solution to this as a smooth gradual transition between cold and warm white throughout the day was the primary goal I set out to achieve.

Regarding the state fields, is there a reason they wouldn't be known when they are being adjusted by either the web gui or via mqtt? Could this possibly be related to the problem above if the esp8266 is not remembering the last state?

Thanks for your time with this.

Temperature adjustment packets:

rgb_cct packet received (9 bytes):
Raw packet: 39 7E 3B 9F 7F 75 40 82 9E 

Decoded:
Key      : 39
b1       : 20
ID       : 0D3F
Command  : 03
Argument : EC
Sequence : 17
Group    : 01
Checksum : F4

rgb_cct packet received (9 bytes):
Raw packet: D4 FF C2 12 48 62 86 4A CD 

Decoded:
Key      : D4
b1       : 20
ID       : 0D3F
Command  : 03
Argument : D2
Sequence : 18
Group    : 01
Checksum : F6

It has just triggered that these are showing as rgb_cct packets despite not being colour bulbs. They are FUT011 and I assumed CCT would be the correct mode. I do have some FUT019 but I was never able to link them to this gateway (I think it's older than v5) and I was only able to link them to a remote (FUT007?).

jstans commented 6 years ago

Ok, so these FUT011 bulbs clearly work on both the rgb_cct and cct protocols with the former seemingly being preferable (despite not having colour) whereas the FUT019 are presumably cct only. Apologies for any confusion this caused. Annoyingly though this means I can't really use the remote any more since it'll not sync the same state between cct/rgb_cct.

Temperature also seems to now be persistent, suggesting that it wasn't correctly saving with the state for CCT?

sidoh commented 6 years ago

As you pointed out, those are RGB+CCT packets, which definitely does have direct-set commands for both brightness and color temperature.

You can probably set up a forwarder for your remote. There's some documentation here.

I'll take a look at CCT's state.

EDIT - You're right, state for CCT is not getting persisted. It might be because keeping track of it incrementally is too messy, I don't recall. I'll take a look.

EDIT2 - Yeah, definitely why it doesn't work. To limit the number of places that state is mutated, state updates are only handled in the "received RF packet" handler. This means that state handling is never aware of the intention to set a field to a particular value. It only sees the increment commands streaming in.

One solution would be to make state handling aware of the global command. But then state for commands sent by remotes or other devices would not be tracked.

Going to take a stab at keeping some scratch state for this.

jstans commented 6 years ago

Apologies again. I was just thrown by the CCT bulbs supporting the RGB+CCT mode. Might it be worth adding a note to the README.md in case someone else does the same?

Will take a look at the forwarder.

I was just taking a look at the code right now and was testing with this, unfortunately I can't even get past the isSetKelvin() working currently so no idea if this is even workable:

void CctPacketFormatter::updateTemperature(uint8_t value) {
  GroupState ourState = this->stateStore->get(this->deviceId, this->groupId, REMOTE_TYPE_CCT);

  if (!ourState.isSetKelvin()) {
    valueByStepFunction(
      &PacketFormatter::increaseTemperature,
      &PacketFormatter::decreaseTemperature,
      CCT_INTERVALS,
      value / CCT_INTERVALS
    );
    ourState.setKelvin(value);
    return;
  }

  uint8_t lastValue = ourState.getKelvin();

  if (value < lastValue) {
    for (size_t i = value; i < lastValue; i += CCT_INTERVALS) {
      this->decreaseTemperature();
    }
  } else {
    for (size_t i = lastValue; i < value; i += CCT_INTERVALS) {
      this->increaseTemperature();
    }
  }

  ourState.setKelvin(value);
}
sidoh commented 6 years ago

Ah, good point. I guess it could go there too.

I'd like to limit the number of places state is mutated, though. Really the stateStore available to the packet formatters should be read-only.

Definitely worth documenting it somewhere. I'm pretty sure this exact thing has come up before. Could you help me consolidate the information we should add?

jstans commented 6 years ago

I am not familiar with the code at all and just wanted to see if I could come up with something workable (which apparently I couldn't), It was certainly not meant to meet any coding standards!

I was just thinking of adding FUT011 to the RGB_CCT bulbs on the README, but that may not get picked up as some people may not even know the FUT number. Perhaps a simple note along the lines of:

Just testing the Mi Light forwarder now and it seems to be a workable solution although I've run in to yet another issue (it never ends!). It seems when turning the lights off it's not updating the group id and instead sticking with the last known group id. A on command is needed to get it to update the group id. so: 1: on, 2: off, 2: on, 2: off, results in: 1: on, 1: off, 2: on, 2: off (edit: the packet sniffer also seems to pick up the incorrect group id in b1)

sidoh commented 6 years ago

Think CCT's brightness/kelvin state behavior should be fixed, or at least better, with 1.7.0-dev8.

My memory is fuzzy, but I think others have noticed this as well. I don't exactly remember where that landed. As you noticed, I think the remote just sends dumb packet values. Sniffed packets are unprocessed, so probably not something we can change.

Can you post some packet logs?

jstans commented 6 years ago

Thanks, that's a much bigger commit than I was expecting! I will run some more tests tonight as well as provide some packet logs.

My tests yesterday showed that the captured packets definitely appear to be sending the wrong group id in b1 for an OFF command. But the remote would be turning off the correct bulb and not the one in b1, maybe it could be using different bits to determine the group id?

jstans commented 6 years ago

@sidoh State seems to be working brilliantly with cct now, thanks for that!

Regarding the CCT incorrect group id when sending an off command from the remote, I have run some more tests and I am not sure what else to say, the packets picked up by the sniffer clearly indicate the wrong group id is being transmitted. But the bulbs are acting as if it's receiving the correct group id.

What I've also noticed is turning 1 or 2 on followed by 4 off results in an off packet with that group id showing up. But turning 3 on followed by 4 off results in no packet showing up in the sniffer at all and no LED flicker from the esp8266. Though the bulbs still turn off as expected.

Certainly appears that there are some hidden packets happening here that the esp8266 won't pick up?

sidoh commented 6 years ago

I think you're right; it's probably interpreting different parts of the packet.

Actually I think the RGBW packet formatter is already doing something different:

    // Determine group ID from button ID for on/off. The remote's state is from
    // the last packet sent, not the current one, and that can be wrong for
    // on/off commands.
    bulbId.groupId = GROUP_FOR_STATUS_COMMAND(command);

Probably the same thing needs to happen for CCT.

jstans commented 6 years ago

Yeah, it looks like Byte 3 is always correct (not sure how I missed this!) and you've already used this with cctCommandIdToGroup() but have not updated the group id against the packet.

sidoh commented 6 years ago

Merged your PR. Gonna close this ticket, but please feel free to re-open if I'm missing something.