search

openshwprojects / OpenBK7231T_App

Open source firmware (Tasmota/Esphome replacement) for BK7231T, BK7231N, BL2028N, T34, XR809, W800/W801, W600/W601, BL602 and LN882H
https://openbekeniot.github.io/webapp/devicesList.html
1.46k stars 272 forks source link

BP5758D SetCurrent doesn't work right for currents above 63 #286

Open dwildstr opened 2 years ago

dwildstr commented 2 years ago

Someday I suppose I should get a clue about how to push to git workflows so I can open these code submissions as pull requests rather than issues, but today is not that day.

The BP5758D uses a slightly strange byte-encoding when setting current. The high bit is ignored, and the low 6 bits are (as might be expected) of value 1, 2, 4, 8, 16, and 32, so currents up to 63mA can be passed uncritically as bytes. But the second-highest bit is not the expected weight of 64, but of 30 (see pages 8 and 9 of the datasheet). So currents from 64 to 93 need to have 34 added to them to have the "right" bit representation for a BP5758D. Currents above 93 can't be represented at all (and aren't supported as valid currents by the chip), and I wouldn't presume to say whether that should be some sort of error, or just silently capped at 93.

In any case, the obvious place for a fix is line 73 of driver/drv_bp5758d.c, changing it to

g_chosenCurrent = (curVal>63) ? (curVal+34) : curVal;

which doesn't really work when curVal exceeds 93 correctly (it'll roll over into the ignored high bit). An additional conditional on curVal>93 could do whatever is deemed best behavior.

openshwprojects commented 2 years ago

This is of course a good change, but I wouldn't call the original behaviour a bug or say that it "deoesn't work", because it was assumed that user enters a correct value from datasheet, and not a numerical value of current.

I can add that fix you made, but what about the other I2C LED drivers, don't they need the same or similar approach?

dwildstr commented 2 years ago

Fair enough on the distinction between not-working and working intuitively; certainly currents from 64 to 93 are obtainable with the established behavior, and I suppose the established behavior is good if someone wanted (for some esoteric reason) to send the BP5758D specifically the unconventional codings of numbers from 30 to 63, but this change will forestall the question "why does setting the current to 64mA make my lights half as bright as 631mA?".

With regard to other I2C drivers, are there chips other than the SM2135 fully supported at the moment? AFAICT, the SM2135 driver in OpenBeken doesn't have amperage control, but is hardcoded to 20mA. I'm not even sure it's hardcoded correctly (based on the datasheet, it seems one should send the byte 0x33, not 0x03) but have no SM2135-driven bulbs to test that on.

If you did want a SM2135 amperage encoding formula, it seems like subtracting 5, then dividing by 5 (rounding the result) and multiplying by 17 is correct (that's the part I'm least certain on --- SM2135 seems to expect a single byte encoding an RGB current in the high nybble and a white current in the low, so to change all currents, you multiply by 0x11).

I looked a bit at what datasheets I could find on the BP1658CJ and couldn't find much information at all about them and none about the protocol for setting current, but that code's very new on OpenBeken and it doesn't have any sort of current control either.

openshwprojects commented 2 years ago

I have added your suggestion to the codebase: https://github.com/openshwprojects/OpenBK7231T_App/commit/971ac49c55e8613ea59b91c32c5df1feba502524 The only drawback is that if someone had it configured in the oldest version, he will have to reconfigure...