UART plugin recovers granularity if timing improves - closes #1798

[markus983]

Hi,
with these changes the UART plugin can recover the granularity if timing improves, similar to how the FTDI plugin solves this issue.

[markus983]

I took care of the lint issues and the PR is now targeting the branch 0.10

[peternewman]

I'm not sure if this was my error or yours, but there's one outstanding lint issue remaining if you wouldn't mind.

Have you re-tested on the 0.10 branch?

[peternewman]

Also your Git-Fu is clearly much better than mine getting it targeting 0.10 branch, well done!

[markus983]

I'm not sure if this was my error or yours, but there's one outstanding lint issue remaining if you wouldn't mind.

It should be fixed now.

Have you re-tested on the 0.10 branch?

Yes I did some testing on the 0.10 branch, and with these fixes, OLA behaved as expected.

[peternewman]

I'm not sure if this was my error or yours, but there's one outstanding lint issue remaining if you wouldn't mind.

It should be fixed now.

Great thanks. For future reference you can also merge them directly from the web UI:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request#applying-suggested-changes

Have you re-tested on the 0.10 branch?

Yes I did some testing on the 0.10 branch, and with these fixes, OLA behaved as expected.

That's brilliant news!

[peternewman]

Thanks @markus983 . Unfortunately I've just spotted an issue with how this calculation is mapped across to this code from the FTDI plugin...

You probably also want to add yourself to Authors for your contribution:
https://github.com/OpenLightingProject/ola/blob/0.10/AUTHORS

[peternewman]

Sorry I've just spotted this calculation won't be right for this plugin.
malft is Mark After Last Frame Time

The code you've copied it from is:

ola/plugins/ftdidmx/FtdiDmxThread.cpp

Lines 86 to 87 in 966c85b

	int frameTime = static_cast<int>(floor(
	(static_cast<double>(1000) / m_frequency) + static_cast<double>(0.5)));

Which has a default frequency of 30:

ola/plugins/ftdidmx/FtdiDmxPlugin.h

Line 66 in b215c65

static const uint8_t DEFAULT_FREQUENCY = 30;

Which gives:
=floor(1000/30)+0.5
=floor(33.33)+0.5
=33+0.5
=33.5

Whereas the input we've got here is the break time and the mark after last frame time (both in microseconds). So the frame time is
break time+(number of slots*slot time)+mark after last frame time

Your calculation will always be 0.5 with the default MALFT.

The standard is available here (E1.11):
https://tsp.esta.org/tsp/documents/published_docs.php

But essentially the break to break timing should be between 1204 microseconds and 1 second.

The standard also defines a minimum update time for 513 slots of 22.7 milliseconds (that appears to disagree with the minimum bit time from my calculations, but it's in the right ballpark).

So I suspect the easy fix here would be to find a frame time based on the minimum time for 513 slots plus the configured break and MALFT times. The nicer option would be to consider the number of slots being sent, but in both cases we should probably honour the minimum break to break time too, so we don't send too fast!

Which essentially is the conclusion that @richardash1981 and @rewolff reached in #1087 !

[peternewman]

As above this value, or the calculation of this value, needs changing...

[peternewman]

And again.

[rewolff]

Just to be sure: The "write" returns immediately. So a working solution should probably get a timestamp at the moment the BREAK condition is released (A scheduling delay during the break then won't affect us), Then issue the write and calculate how long that should take, add in the mark-after-last-frame and wait until that moment before starting the next cycle.

When I was looking at the code, the person writing the code assumed that write would return when all bytes had been written. My suggestion above does not depend on whether the write returns immediately or not.

[markus983]

@peternewman Thanks for the info, I fixed the calculation of the frame time and it should honor, the boundaries now. I tested it as well, and the UART plugin still behaves as expected. Personally I don't like goto statements so I just split the code into more methods, but it still behaves the same (and it helped me with debugging).

@rewolff Thanks for your input, I incorporated this in my newest changes.

[rewolff]

Glad to have been of help. I haven't been "intimate" with the code in a while, so good to hear that my "research" from a while back wasn't wasted and helped the actual code. :-)

[peternewman]

Sorry for ignoring this for a bit, all sorts of stuff has been going on and I've been fighting the good fight with Python.

I'm hoping we can still sneak this into 0.10.9 if you've got a bit of time to just tweak it a bit?

[markus983]

I tweaked the code as you've suggested - thanks for the feedback! Hopefully, this PR can be included in 0.10.9.
Unfortunately, it looks like I've somehow messed up the git history of my forked repo. I was able to push my code to the correct branch in the end, I just hope that this didn't mess up anything else.

[peternewman]

I tweaked the code as you've suggested - thanks for the feedback! Hopefully, this PR can be included in 0.10.9.

🤞

Unfortunately, it looks like I've somehow messed up the git history of my forked repo. I was able to push my code to the correct branch in the end, I just hope that this didn't mess up anything else.

Yeah that's about what normally happens with me too!

Do you want to open a PR against this which I think is what you actually intend to merge now?:
0.10...markus983:ola:0.10

[peternewman]

As mentioned can we have the PR from your 0.10 branch to our 0.10 one and some other fixes to add to it too.

[peternewman]

FWIW, although we've just cut 0.10.9, I'm planning to roll out 0.10.10 within the next few days ideally, or few weeks if a few key outstanding PRs like this one get resolved and merged.

[markus983]

Do you want to open a PR against this which I think is what you actually intend to merge now?:
0.10...markus983:ola:0.10

Yeah, that would make it easier. The "new" PR is now here: #1825