Improved LED Smooth Transition function

[rogere66]

With the introduction of LED gamma correction and calibration the smooth transition function got some new issues. This PR corrects these issues and generally improves transitions. To get it working the lerp function is now applied on the channel values prior to brightness and gamma correction, and separately on channel and brightness values. Brightness also implements soft start and stop lerp for even smoother operation.

The lerp function was previously implemented in a separate function that was very similar to the apply_smart_light() function - this is now merged into a single function, which should make it easier to maintain and add new features. I have tested the code in the different PWM operation modes, but don't have any LED's with external PWM chips so this is only tested by simulation.

I also disabled the MQTT Log messages by default. When the messages are enabled the LED transitions can get quite choppy. The root cause of the problem is that the log message UART code is blocking the OS while sending the message - this is a separate issue which should also be looked into. The UART messages can also be disabled with the "logtype none" command. Maybe all the UART Log Messages can be disabled after Boot Complete?

[openshwprojects]

Great improvement but right now has a single drawback - I think that this " (0 << 1) | // disable MQTT by default (due to blocking UART)" disables all logging of MQTT messages, and not just the UART one. This filtering applies to web app log as well.

Does the UART problem also occurs with direct log?

I think we could also solve the problem by disabling UART log (but only UART?) when LED smooth transitions are enabled and safe mode is not active... and maybe also add a flag for that, something like "Disable UART logging when LED smoothing is on" that is enabled by default

Or maybe just the getSerial2 could be modified to send in smaller chunks? @btsimonh , can you look into that? @rogere66 could you test how it behaves with:
a) getSerial2 doing nothing
b) getSerial2 having some kind of imposed limit of 64 characters call (in the while loop), or 32...

[rogere66]

Yes, the UART problem also occurs with direct log - it is even worse (using command "logtype direct").

a) getSerial2 doing nothing: no glitches
b) getSerial2 having some kind of imposed limit: still glitches sending 32 chars + rtos_delay_milliseconds (10)

The problem may rather be in the filling of the logMemory buffer. In addLogAdv() this write loop seems suspect - what happen if the buffer get full?: Edit: No, there is no problem here - the buffer will just be overwritten if full.

for ( i = 0; i < len; i++)
{
	logMemory.log[logMemory.head] = tmp[i];
	logMemory.head = (logMemory.head + 1) % LOGSIZE;
	if (logMemory.tailserial == logMemory.head)
	{
		logMemory.tailserial = (logMemory.tailserial + 1) % LOGSIZE;
	}
	if (logMemory.tailtcp == logMemory.head)
	{
		logMemory.tailtcp = (logMemory.tailtcp + 1) % LOGSIZE;
	}
	if (logMemory.tailhttp == logMemory.head)
	{
		logMemory.tailhttp = (logMemory.tailhttp + 1) % LOGSIZE;
	}
}

[openshwprojects]

Would running quick tick or lerp less often at least partially mitigate the problem?

By the way... you just made me realize... the IR timing mismatch may be also caused by that log problem!

I will rethink this and for now at least try to add some kind of log-disable mechanism in a clean boot or smth...

[rogere66]

@openshwprojects I think I may have found the problem: It looks like getSerial2 is called from the timer thread and then is always called before QuickTick, delaying QuickTick until the log buffer is empty. I tried to run QuickTick as a thread and also insert a rtos delay call in getSerial2 (to do re-scheduling) and then it works fine. It will probably also work fine if getSerial2 is run as a task. What do you think?

[rogere66]

I have tested this further and it is clear that it is the current use of timers that cause problems. All timer callback functions run in a single thread and will thus delay each other. Basically the callback function should return as fast as possible and not do significant processing or blocking.

For periodic tasks it is much better to run each in a separate thread, typically using the vTaskDelayUntil() rtos call. For QuickTick() it will look like this:

void quick_timer_thread (beken_thread_arg_t arg)
{
	while (1) {
		TickType_t currentTickCount = xTaskGetTickCount ();
		QuickTick (0);
		vTaskDelayUntil (&currentTickCount, QUICK_TMR_DURATION / portTICK_PERIOD_MS);
	}
}

I have tested this for QuickTick() and Main_OnEverySecond() and it all works great. I can update the PR if this is an acceptable solution.

[openshwprojects]

I haven't tested it yet, but according to my knowledge we had a serious issue where device, usually while using MQTT, was crashing about after few days of use. Have you done enough testing to make sure that firmware is stable after that change? It would be good if @btsimonh or someone else also reviewed it.

Moving all MQTT-capable functions to timer thread was to make sure that everything is done is a thread-safe way and works well on all platforms.

Good test to do is a MQTT benchmark function from new_mqtt.c. Another thing worth testing would be an IR remote spam (press and hold button on press many button repeatedly with IR MQTT publish enabled).

[openshwprojects]

I am wondering if it would be safer and easier to call UART log dump after each quick tick (from within quick tick) with some kind of imposed limit of characters to be processed so it won't spend too much time there

[rogere66]

@openshwprojects I don't think it will help much to move UART log dump to QuickTick as long as they both run in the timer thread - that is almost the same as now.
I have tested it quite a bit and believe it will work, but have obviously not covered all possible corner case. What if we create a QuickTick2 that includes everything except the MQTT stuff? - MQTT would then still be on the timer thread. As a minimum solution it is also possible to move only the lerp function to a separate timer thread.

It is however clear that the timer thread is not used as intended by FreeRTOS and it is quite possible that it cause other problems too. As the code is now there will occasionally be delays of up to 300-400 mS on any timer task.

[rogere66]

@openshwprojects Have re-based the code and moved the lerp function to a separate timer thread. This should make the lerp function work well with no significant impact on other functions. The issue with the timer thread should be handled separately.

[openshwprojects]

I will try to look into it in the following days. The issue here is that we can't afford to worsen stability for our users, including of course BL602 users as well. I must find a day or two to test your changes well. Do you happen to have W600/W800/BL602 dev board to help us with testing?

Or if anyone is reading here and can help testing, just me know. It's always better if more people test. This change + mqtt benchmark ran in the background to see if it's stable in long term.

[rogere66]

@openshwprojects I understand your concern regarding different targets - unfortunately I only have BK7231T devices.

If the main concern is the changes in user_main.c I suggest to leave this out for now. The important part of this PR is the greatly improved lerp functionality - this will be a large improvement even without fixing the QuickTick delays.

[rogere66]

Not much happening with this so I will just wrap up my contribution by listing some issues I have seen while working on the code:

1. The function rtos_create_thread() used for BK7231 reverse the priority setting compared to calling xTaskCreate() directly. It may be intentional, but may also be some legacy stuff. For the other processor types the priorities are not reversed and also set different than for BK7231. I used the FreeRTOS vTaskList() function to display the tasks priorities, and I would say the priority settings looks a bit odd - at least the timer thread priority is part of the problem with blocking UART.

2. As mentioned earlier the current use of timers is not optimal. All timers run in a single thread/task without a defined priority between the different timers. At least for the QuickTick and Second timers it will be far better to run them in separate tasks.

3. After the LED gamma update the Channel Start value settings don't work anymore, and is not really relevant either since it don't include brightness. To set the LED start values they should be set in the same way as when setting Flag 12 "[LED] Remember LED driver state". A simple fix would be to add a flag and/or button to modify the Flag 12 function such that the start values are not updated for every LED update, but only when requested.

4. In web-GUI Config/Configure MQTT it is not possible to clear the Group Topic. It can be set to "space" or something unused, but it will still generate an MQTT subscription request.

I have also made a new branch with the code I am currently using (Local_Code). It contains some extensions that might be useful for other users too: some FreeRTOS/system view commands, permanent LED start settings, ADC function publishing MQTT group commands (use potmeter to set brightness for a LED group) and not least, the improved Smooth Transition/Lerp function (files sysView.c and userExtension+).