Question: are you sure changing ola_recorder is a good idea ?

[Jeff0S]

For the moment, we stick to 0.10.7 so everything is ok but we had a look at what is going on in the master, and I must say I'm not too fond of the recent changes made to ola_recoder, feature-wise and code-wise...

Not sure adding this start/stop thing will be THAT useful, but hey, why not. However, and it's more annoying, the design changed, code overhead was introduced (normal with such a feature: many corner-cases to handle), etc.
=> At the end of the day, we cannot keep up-to-date with OLA + cherry-pick our own mods to extend ola_recorder anymore (w/o huge conflicts, I mean) and I guess we won't be the only guys in this case.
=> Even more annoying (the damn thing being in production), we'd have to re-code/re-test everything for a feature we aren't interested into.
=> And, then, in the future, the whole thing could yet happen again...

Although they're functional and (VERY) useful, I would tend to think these ola/examples are just made to get coders started with writing OLA clients. I would keep these examples dead simple, or at least, I wouldn't touch them so radically (one can branch/cherry-pick/patch things to extend them just like we do, or contribute some sort of "extended" but -distinct- player if he really wants to).
IMHO, these examples should only benefit from bug fixes, or small features (i.e. no impact on the design).

On our end, the "fix" will be quick/easy (either reverting or copying things as they were in 0.10.7), but that would be silly since our ola_recorder would then live "out of project" (e.g. won't benefit from bugfixes anymore, etc).

^ that's why I thought I should ask first: are you sure changing ola_recorder is a good idea ?

Regards,
Jeff

[peternewman]

Hi @Jeff0S ,

Can I ask what mods you've done to ola_recorder? The easiest way to resolve this issue would be to open a PR and get them merged in, then you wouldn't need to deal with conflicts (although you may still wish to test it).

I'd say the same for anyone else who has made local mods, please merge them in, then everyone benefits and you don't have to deal with the maintenance burden alone.

We've had at least two users ask about in/out points and you're the first person to mention not wanting them, so I think there is a desire for them. Whereas you seem to be asking for no changes to be made at all, which is rather untenable.

I think looking at the changes, relatively speaking there will be very few changes to the code path if you're not in simulate in this new version of the recorder. I've also tried hard to ensure that calling it from the CLI it should be backwards compatible. We've also fixed a number of bugs and edge cases with the existing implementation.

In terms of your production code, surely you need to re-test whenever olad itself changes, although we try hard not to introduce new bugs, we only properly test the library parts of the codebase and not the interactions between them, so it's possible for new errors to appear. Aside from bugfixes and support for new protocols I'd wonder if you'd be better off sticking with a fixed release if you don't want to have to re-test.

I think it's perhaps better to think of these examples as examples of how to interact with and test the API, rather than examples of how to programme using it. If you look at ola_client for example it does lots of different things and they're all mixed in together that it's not very easy to find the bits of code you need just to send DMX. In comparison, the Doxygen examples just do one single task in a way you can easily modify for your code:
https://github.com/OpenLightingProject/ola/tree/master/doxygen/examples

IMHO, ola_recorder should perhaps never have been in there anyway, and should probably have been moved off separately like ola_trigger as it has a bit more potential than just CLI equivalents of the web interface although it also shows the power of the olad core that a handful of lines of code can give you full playback and recording:
cdc569d#diff-8876d59c8a1e0bf48ba47fc24830c282

You could probably say the same of the rdm_compare and rdm_snapshot python examples.

From the feedback I've got from other people they'd actually like ola_recorder to be far more fully featured for example there is already an OSC interface wrapper ( https://github.com/Lightsteed/OscOlaRecorder ) adding a HTTP one (or a direct OSC one) would be fairly easy and would simplify automation. Some of the commercial recorders allow you to trigger off a DMX channel or universe and again that would be easy and beneficial for people. Having two sets of patching for play and record and a simple way to change between them. Ultimately the code for that is all very simple, the UI and options are as always the challenge.

I think in some ways ola_recorder is perhaps OLAs best kept secret, and if some of that stuff was wrapped up into a nice user-friendly web interface, places like museums could save lots of money on buying some commercial bit of hardware. It could also be a completely standalone project that just uses the OLA API.

Thanks for your feedback though, as you might have seen in the PR we had some debate about backwards compatibility and whether people might be using bits of the code directly, so it's good to get some indirect feedback on this. Also please do let us know if you find any bugs with the changes and we can get them fixed.

In terms of your code, the better thing to do might be to separate it out from the main OLA codebase. Clients don't need to be within the codebase (see for example QLC+) then you could just maintain your fork separately (if you don't want to merge the changes back in), and each time there is a release update olad but keep your code as is, then check for any changes to the ola_recorder code and see if they apply to your version too.

P.S. you probably won't like my other changes to it in #1607 either then sorry (although they are at least more minor).

[Jeff0S]

Thanks for this thoughtful reply, Peter!
I've learned things on the way!

ola_recorder: Ok, I'm not too surprised...
As I said, this isn't a biggy for us. I just wanted to make sure something wasn't overlooked here, since those examples haven't been touched in years...

About the PRs:
You're right of course! This isn't due to some sort of "bad will" though: we're just burning the clock... Having been myself the lead of a popular open-source project, I know these kind of things can be time consuming but we really have an effort to do about that... or we'll end up burning more clocks, just like now :)
As discussed, the recent ola_recorder changes will make it difficult (time consuming) for us to contribute but, we can probably do it in the "0.10" branch. And, now that we MUST switch to a new HW for new features, it's the good opportunity to upgrade/recompile/re-test everything. I'm evaluating if we should go for the master branch (vs the "old" 0.10.7, or vs 0.10 branch), and if it is the case, we'll be able to contribute with extended tests, at least => in any case, you might hear from us in the coming weeks!

About your question:
I didn't double-check in the code, but on top of my head we have mods ranking from small bugfixes (one I can remember of, is the player hanging vs empty show files in 0.10.3) and small features (a --wait param, a new binary file format for huge scenes vs RPi's SD cards, handling SIGTERM to stop the player properly -- because it can be forked/killed in some of our setups) to bigger features like "masks" (playing a file but only for some channels) or cross-fades between cues (smooth transitions with fade from the current DMX state to the state of the 1st frame of the show file).

"ola_recorder is perhaps OLAs best kept secret" :
I agree, it's awesome! Here's ola_recorder in action (lighting HW hand-crafted by ereimul): https://vimeo.com/232822195/d209cd443b

[peternewman]

Thanks for this thoughtful reply, Peter!
I've learned things on the way!

No worries, thanks for raising it in the first place.

ola_recorder: Ok, I'm not too surprised...
As I said, this isn't a biggy for us. I just wanted to make sure something wasn't overlooked here, since those examples haven't been touched in years...

Most of them have had odd tweaks, but probably nothing as significant, but that's mostly because they don't actually need many changes or there aren't that many fixtures you could add to them. Most of the benefit is in new plugins to support new protocols or some minor internal tweaks to deal with new datatypes in RDM.

About the PRs:
You're right of course! This isn't due to some sort of "bad will" though: we're just burning the clock... Having been myself the lead of a popular open-source project, I know these kind of things can be time consuming but we really have an effort to do about that... or we'll end up burning more clocks, just like now :)

Yeah understood, I wondered if it might be something like that.

As discussed, the recent ola_recorder changes will make it difficult (time consuming) for us to contribute but, we can probably do it in the "0.10" branch. And, now that we MUST switch to a new HW for new features, it's the good opportunity to upgrade/recompile/re-test everything. I'm evaluating if we should go for the master branch (vs the "old" 0.10.7, or vs 0.10 branch), and if it is the case, we'll be able to contribute with extended tests, at least => in any case, you might hear from us in the coming weeks!

Master is mostly new features, most of the bugfixes should be in 0.10 too, although as mentioned, you could always go to master and keep your version of recorder in a separate repo. Okay we'll wait patiently then. 🤞

About your question:
I didn't double-check in the code, but on top of my head we have mods ranking from small bugfixes (one I can remember of, is the player hanging vs empty show files in 0.10.3) and small features (a --wait param, a new binary file format for huge scenes vs RPi's SD cards, handling SIGTERM to stop the player properly -- because it can be forked/killed in some of our setups) to bigger features like "masks" (playing a file but only for some channels) or cross-fades between cues (smooth transitions with fade from the current DMX state to the state of the 1st frame of the show file).

Those features and fixes all sound very interestingly. Clearly I'm biased, but I'd suggest commit them somewhere and push them up as a merge against master, just ignore the conflicts but get the code available! I suspect the empty file, wait and SIGTERM could be pulled in by someone (either others or yourself) within an hour or two, probably less. Fades are something someone else has mentioned when discussing it. Masks would probably go in #280 (so you could go ola_recorder->mask->universe (merge)->output); there's no reason they couldn't be implemented as standalone client apps for now, which would mean the code would be ready for future integration. Someone else has been working on playback on a microcontroller, so using a common binary file format everywhere would obviously be beneficial. What sort of times were you dealing with?

"ola_recorder is perhaps OLAs best kept secret" :
I agree, it's awesome! Here's ola_recorder in action (lighting HW hand-crafted by ereimul): https://vimeo.com/232822195/d209cd443b

That's cool! Might make it rather hard to eat your dinner though! 😆 Are the lights actually moving or just lots of individual lights in a line being switched in sequence?