Rewritten the python runners to be more robust and add FRAME_TIME column

[gentlegiantJGC]

This rewrites the python run.py and run-folder.py quite dramatically.

Previously the CSV parsing code assumed that the columns were positional which meant adding more columns was impossible and would break if the colums were not in the "correct" order. This version reads the headers to find where they are. It will also populate any missing column with default values.

It also adds an optional FRAME_TIME column so that the user can specify how long each frame should remain for. If this is not defined it will run the frames as fast as the hardware will run them.

Fixes #2

[gentlegiantJGC]

This is still WIP but I thought I would open it now so that anyone who wants to review it early can and suggest changes.

[melroy89]

I will start reviewing this, thanks!

[gentlegiantJGC]

I think this is ready to review now

[melroy89]

I see a lot of duplicated code between the run.py and this run-folder.py. Could we not just create a library file, which we import in both files? So, when we find a bug, we only need to change 1 file.

Otherwise, there will be soon an issue with different versions of the two files.

[gentlegiantJGC]

Yes it absolutely can. I was just designing this for the case where someone may just want to download and run a single file.

[gentlegiantJGC]

There was already a large amount of duplicated code between these two files so I assumed they wanted it runnable as a single file

[melroy89]

@garciadelcastillo Is this true, or should we avoid duplicated code? And make it better..

[gentlegiantJGC]

I have moved all of the duplicated code and some other utility code to run_utils.py
This includes the csv parsing code and the frame drawing code
3368a05#diff-2225fc874edd373fea2965452dc470310944e6e05210c38d0d4b6f48f644e03e

[melroy89]

Great!

[Diggsey]

It might make more sense to specify this in milliseconds.

[Aonodensetsu]

I second the millisecond proposal

[gentlegiantJGC]

Okay I will change this

[gentlegiantJGC]

Just changed it

[Diggsey]

These types are very deeply nested. You could:

• use a namedtuple for the outer tuple to make it clearer.
• Alias Tuple[float, float, float] as Color (maybe use a namedtuple for this if it doesn't cause problems elsewhere too)
• Alias List[Color] as Frame

This would result in the type signature being significantly simpler.

[Aonodensetsu]

My visualiser returns List[List[List[float]], List[float]] so there's not much simplification to be done about this unfortunately but aliases seem like a good way to make this more readable

[gentlegiantJGC]

The latest commit resolves this
3368a05#diff-2225fc874edd373fea2965452dc470310944e6e05210c38d0d4b6f48f644e03eR9-R14

[Aonodensetsu]

that doesn't match with the top of the README now, there it explicitly states FRAME_ID then FRAME_TIME

[Aonodensetsu]

are we trying to be backwards-compatible again?
because in one place the old code has been mentioned as being discarded

[gentlegiantJGC]

The part at the top doesn't mention order at all. In theory the columns could be in any order and it should run just fine. The new code does support that but for compatibility with the old code it should be formatted like that.

The old code does not allow columns to be added so the only options we have are to either replace the unused FRAME_ID column or break the old code.
Discussion for this can be found here and the few comments below that.
#2 (comment)

[Aonodensetsu]

okay then, changed format for my visualiser to that one, FRAME_ID isn't useful anyway, i might write a converter

[gentlegiantJGC]

Note that I have changed FRAME_TIME to miliseconds as others suggested

[gentlegiantJGC]

I have done a bit of testing of time.sleep and found that it isn't actually as accurate as we really need for this.

A workaround is to go into loop that does nothing to delay more accuratly.

while time.perf_time() < end_time:
    pass

[Aonodensetsu]

class Clock:
  def __init__(self, fps):
    self.start = perf_counter()
    self.frame_length = 1/fps
    @property
    def tick(self):
      return int((perf_counter() - self.start)/self.frame_length)
    def sleep(self):
      r = self.tick + 1
      while self.tick < r:
        sleep(1/1000)

[gentlegiantJGC]

class Clock:
  def __init__(self, fps):
    self.start = perf_counter()
    self.frame_length = 1/fps
    @property
    def tick(self):
      return int((perf_counter() - self.start)/self.frame_length)
    def sleep(self):
      r = self.tick + 1
      while self.tick < r:
        sleep(1/1000)

Testing on windows gives about a 10ms error with time.sleep()

def test():
    import time
    for _ in range(10):
        start_time = time.perf_counter()
        end_time = start_time + 33/1000
        while time.perf_counter() < end_time:
            pass
        print((time.perf_counter() - start_time)*1000)
        
test()

33.00139999998919
33.0022999999926
33.0016999999998
33.00149999998325
33.0016999999998
33.026400000011336
33.001299999995126
33.001599999977316
33.001799999993864
33.001299999995126

def test():
    import time
    for _ in range(10):
        start_time = time.perf_counter()
        end_time = start_time + 33/1000
        while time.perf_counter() < end_time:
            time.sleep(1/1000)
        print((time.perf_counter() - start_time)*1000)
        
test()

47.46529999999893
47.912599999989425
47.909200000020746
48.190000000005284
45.69240000000718
48.07239999999524
49.216999999998734
46.702799999991385
47.99470000000383
47.8847999999914

Edit: apparently the smallest sleep time on windows is about 10ms which is about what we are seeing.

[Aonodensetsu]

that's pretty bad then, i thought python could do 1ms sleeps, or was that some module? hmm

Edit: people are pulling about 5 microseconds with sleep(1.9e-7) on a Pi so it should be possible, maybe windows is just bad

Edit2: well, since there's a solution i'll just implement that, no reason not to

[gentlegiantJGC]

pass on a loop does work but seems to block other threads (I have a matplotlib visualiser on a different thread so this isn't ideal)

Just found that this yields to the other thread and does not have the 10ms issue

while time.perf_counter() < end_time:
    time.sleep(0)

[Aonodensetsu]

ah yes, the notorius 'sleep but not really', i use that all the time in Automate as delay padding

[Diggsey]

Edit: apparently the smallest sleep time on windows is about 10ms which is about what we are seeing.

Yeah the default timer resolution is 10-20ms. You have to opt into higher resolution using timeBeginPeriod/timeEndPeriod: https://docs.microsoft.com/en-us/windows/win32/api/timeapi/nf-timeapi-timebeginperiod

[gentlegiantJGC]

Edit: apparently the smallest sleep time on windows is about 10ms which is about what we are seeing.

Yeah the default timer resolution is 10-20ms. You have to opt into higher resolution using timeBeginPeriod/timeEndPeriod: https://docs.microsoft.com/en-us/windows/win32/api/timeapi/nf-timeapi-timebeginperiod

I don't see a way to configure that for the sleep function in python

[d-albrecht]

Speaking of features that would further reduce file-size: How would your change cope with empty values in defined columns? While optimizing my animation I ran into the situation that LEDs that are off where written as "... ,,,, ..." instead of "... ,0,0,0, ..." and although I fixed this by now, I'd argue that it would be totally reasonable to define empty values as a valid zero value. Alternatively, you could define empty values as a placeholder for "whatever was in this spot the last frame".

[Aonodensetsu]

I think "keep last frame" would be more reasonable since neopixel would by default do that, but it would complicate visualization somewhat, nothing that can't be solved though.

[d-albrecht]

For my animations being able to keep certain pixels unchanged without having to repeat the color codes would only affect the black areas (or to be more precise, I wouldn't bother reducing the file size further by adding a lot of more code just for a size reduction of maybe another 2%), but for any animation that "steps" through a bunch of static images that are then shown for a few frames (although these should use the new FRAME_TIME-feature) or - as I said - for areas with no change at all (for some part of the animation) this could easily half the file size in some cases.

In addition, it would be awesome if the tree would support some compressed animation format (a csv inside some archive-format) as csv is really good compressible and that is what makes uploading some animations difficult. I don't know if python can handle them and if Windows can produce those but gzipped (.gz) files are about one tenth in size and this format doesn't support multiple files, it just compresses one payload, so the python code doesn't have to deal with ambiguous file names in the archive format, as there is exactly one "file"/payload.

[gentlegiantJGC]

I don't think blank values are a good idea. The current code will throw an error as it tries to convert a blank string to a float.

If we wanted we could also support excel formats which are compressed I believe. I assume there is python library to read those but it might be a third party library. I think we should stick with CSV for now though

[gentlegiantJGC]

@range-et @garciadelcastillo what do we need to do to get this merged? It would be great if everyone can use it for Matt's video in early January because currently the frame rate is unknown.

[Aonodensetsu]

yeah the FRAME_TIME is priority I'd say, but empty instead of 0 doesn't do that much, the files are already pretty small and storage just isn't that much of a problem to compress it even more

Edit: I'll think up a compression format specifically for this purpose, I'll report back if it matters later

[d-albrecht]

I almost assumed that the code would break on empty values, but that should be manageable/changeable, right?

ods is also compressed but these "spreadsheet-formats" pack a lot more than just plain cell data and not every spreadsheet application can handle 1500 columns (LibreOffice can't). Therefore, it might be real tricky to produce them and the benefit might be not as great as direct compression. But I agree that this isn't a necessary feature today. And, if we just agree on a compression format, decompression could be handled by the RPi before the python code is even executed. But this would probably be a topic for another issue and not this pull request.

[Aonodensetsu]

I thought up of how to compress csv files (which matt said he will use csv), I'll make a mockup soon on my repo

[garciadelcastillo]

@gentlegiantJGC please see https://github.com/GSD6338/XmasTree#contributions

[gentlegiantJGC]

Okay so you want me to submit the improved code to their repository?

[garciadelcastillo]

Entirely up to you, please coordinate with Matt.

[Aonodensetsu]

I'd assumed that since Matt pointed us here, we should do what is needed here then merge that upstream to him

[gentlegiantJGC]

Okay I was under the impression this was something you wanted to merge.
That text has appeared since I started working on this

[d-albrecht]

Matt's repo(s) don't contain any (csv-based) animation code, therefore, I'm pretty sure he intended to just use the latest version from this repo. But if this repo will no longer be maintained, I guess the two options are (a) create a PR over there with your code or (b) create a fork and link to it in Matt's "Further Work" section.

[Aonodensetsu]

i think the easiest resolution will be for gentlegiantJGC to fork this, merge his own PR into that authoriatively, then change the README of standupmaths/xmas-tree in the main section such that it points to that repo for the authoritative version of the runners

[Aonodensetsu]

so far I've compressed some random csv effect I had locally by about 69% (nice), I'll write the decompression algorythm and upload the file

[d-albrecht]

Hey, if you have joy in working on such compressions, go ahead. Just saying that on my animations (that might just be plain more compressible than your random ones), I achieved (more than) 90% with a tried and true gzip.

[Aonodensetsu]

I have no doubt that my compression might not be the best compression ever, but it's supposed to be able to be decompressed on the fly as the code runs and displays it, encoding and decoding are faster than real-time

[d-albrecht]

Then, this would probably contradict the improvements in the runner code that support reordering the frames when a FRAME_ID column is present as this requires the code to traverse and parse the whole file at least once. If your compression format can support quasi-random access of arbitrary lines then this would be well suited for the new runner script. Otherwise, supporting (de)compression on the fly won't make a difference, sadly.

encoding and decoding are faster than real-time

What do you mean by that? That decompressing one line takes less time than displaying one line? That should be true for almost every compression format. Usually, decompressing a whole animation should take at most as long as animating a hundredth or a tenth of this animation. If (de)compression took too long, the whole concept would not be used. ;)

[Aonodensetsu]

i haven't seen the reordering mentioned since i replaced FRAME_ID with FRAME_TIME, and i think reordering is pretty stupid, why not just send the csv after sorting it? full de/compression takes less time than animating a single frame currently so I guess it's pretty fast, it could be even incorporated into the displaying itself without slowing it down

[d-albrecht]

I agree that there aren't many use cases for reordering. If you could annotate one frame with multiple frame ids, then that would be useful to reduce file size in certain situations, though.

With these new details, your compression really seems pretty fast. But I don't have any experience in this field, so, I can't compare this to other algorithms.

Last time I checked, the current run-folder-script version loads all animations into memory first before even starting to display anything, if I'm not mistaken. But, being able to decompress on the fly would certainly be desirable if the code would work differently. For example, you could design the code such that it animates one pattern, then loads another random file from storage, animates this, and then loops. Such that you could add new files at any time that would be picked up eventually. Like a deamon that never stopped. You could spend several hours in from of such a tree with new animations every few seconds.

[Aonodensetsu]

I agree that annotating multiple IDs to a single row could be useful, didn't think of it. The program should have at maximum 2 files in memory at any point, the currently playing one and maybe the next one if we were to implement lerp as someone suggested. If it could pick up files at random (or alphabetically) then adding them would be as simple as Matt setting up a cron to curl from the examples folder, which would then display all sent effects (a stream showing the tree for a long time maybe).
Maybe the runner could parse the next-in-line animation while the last is still playing to ignore malformed files. I doubt that would be too difficult as we have clear definitions of what each row & column can contain.

My compression has to be fast because it's simple - the same reason why it produces larger files than gzip for example. Gzip is complex, therefore it can put a lot or data into a small size, but it requires some more time to create that file. Also it has error correction while my compression has none - again, easier to de/compress, less protection from transmit errors.

Edit: I think I'll bake checksum into the format itself because some error protection can go a long way and that'll add like 8 bytes to the file size.