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

FT232R: use native Python time.sleep() on Linux #6

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

s-2
Copy link

@s-2 s-2 commented Nov 27, 2024

fixes compatibility with certain libc implementations, e.g. musl on OpenWrt


When trying to use PyDMX with the FTDI driver on an OpenWrt device,
I found it was not outputting any DMX signal, while the same code was working fine on a ubuntu desktop.

As it turned out, there seems to be an issue with the musl implementation of nanosleep as used by OpenWrt,
which made me wonder about the advantage of calling a ctypes function here, rather than using native Python3 sleep().

Regarding performance of the DMX output, I made a few measurements on both desktop and an mt7621-based OpenWrt router, and it did not make any noticeable difference (actually CPython would use usleep or nanosleep anyways, when available), c.f.: https://forum.openwrt.org/t/python3-ctypes-nanosleep-broken-with-musl/216072/11

Could we just change it to use time.sleep() here?
Besides being more Pythonic, at least it made it work on OpenWrt for me 🙂

besides, the original code seemed wrong regarding the use of modulo to split the fractional part off the nanoseconds:

in wait_ms there is

        sleeper.tv_sec = int(milliseconds / 1000)
        sleeper.tv_nsec = (milliseconds % 1000) * 1000000

but in wait_us:

        sleeper.tv_sec = int(nanoseconds / 1000000)
        sleeper.tv_nsec = (nanoseconds % 1000) * 1000

probably should have been

        sleeper.tv_nsec = (nanoseconds % 1000000) * 1000

instead? For this driver it would not make any difference of course, since sleep is never called with full seconds.

fixes compatibility with certain libc implementations, e.g. musl on OpenWrt

Signed-off-by: Sebastian Schaper <[email protected]>
@JMAlego
Copy link
Owner

JMAlego commented Nov 27, 2024

Hi there, thanks for suggesting this.

I can't actually remember why I used the native function but my guess is it's to do with time.sleep's behaviour before Python 3.11 ("Changed in version 3.11: On Unix, the clock_nanosleep() and nanosleep() functions are now used if available. On Windows, a waitable timer is now used."). The exact reason is lost to the annals of history now...

I'll see if I can fish out my hardware for this and take a look at the timing impact. My guess is that nowadays it's probably safe to add a warning for people using pre-3.11 Python but I'll have a little think on it - this weekend probably.

Also good catch on the maths. If I ever got 'round to writing more testing I may have caught it but in this case as it's not actually used I didn't catch it.

@s-2
Copy link
Author

s-2 commented Nov 27, 2024

my guess is it's to do with time.sleep's behaviour before Python 3.11

I see, hadn't checked in the docs how this was implemented in earlier versions, so it might indeed break existing installations when the driver would be updated.

(Besides, I was hoping to get some feedback from OpenWrt folks regarding nanosleep - there might be many other applications using it that don't work - but no one really seemed to care yet...)

@JMAlego
Copy link
Owner

JMAlego commented Nov 27, 2024

This has actually got me interested in the timing on this - as your comments in the linked forum points out, the timing is not quite in line with the spec. My guess is it was fudged but I don't like magic numbers for that so I need to work out why and add some comments at least to explain it.

Like I said above, probably a fun job for this weekend 😆

Oh and a hint, you mention not being able to transmit less but actually there's a switch for that:
https://github.com/JMAlego/PyDMX/blob/8d5ad461dac2fbfb4e6d9397cbea22d9d756bbde/core/dmx/universe.py#L71C5-L71C61 i.e. partial=True.

@s-2
Copy link
Author

s-2 commented Nov 27, 2024

Oh and a hint, you mention not being able to transmit less but actually there's a switch for that:

Ah, thanks. I think I saw something initially, but then must have forgotten about it.

Regarding the timings, I would have expected there to be much more difference between hardware platforms, but between x64 and mips32 it was mainly the MAB period that differed much, might also be within the FTDI implementation.

I might as well check on my phone running SailfishOS 😂

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

Successfully merging this pull request may close these issues.

2 participants