Hello @vtavernier 👋

I'm your friendly neighborhood bot and would like to say thank you for
submitting a pull request to Hyperion!

So that you and other users can test your changes more quickly,
you can find your workflow artifacts here.

If you make changes to your PR, i create a new link to your workflow artifacts.

Best regards,
Hyperion-Project

@vtavernier Sorry, did you test that the proposed changes will fix the problem that effects started via tray do not run?

Here the Booteffect nicely works and is stopped.
If I start an effect via Tray, it runs directly into wrapAbort with getRaimaining = -1 and the effect is not executed....
(I just added some output statements....)

021-03-03T20:59:25.431 hyperiond EFFECTENGINE : <INFO> Run effect "Knight rider" on channel 1
2021-03-03T20:59:25.431 hyperiond EFFECTENGINE : <DEBUG> EffectEngine.cpp:183:runEffectScript() | Start the effect: name [Knight rider], smoothCfg [2]
2021-03-03T20:59:25.431 hyperiond HYPERION     : <DEBUG> PriorityMuxer.cpp:161:registerInput() | Register new input 'System/EFFECT' with priority 1 as inactive
EffectModule::wrapAbort
Effect::isInterruptionRequested(): _interupt [0] getRemaining(): [-1]
Effect::isInterruptionRequested(): _interupt [0] getRemaining(): [-1]
2021-03-03T20:59:25.444 hyperiond EFFECTENGINE : <INFO> effect finished

Would you mind check and test again?
Thank you!

OK. I have the issue. Sometime setEffect() is called without timeout parameter populated which then defaults to -1 and therefore the effect stops immediately as timeout < 0 is met.

Edit: I will take-over from here. Thanks for your support and bringing the initial fix!

Good catch! I also thought something was different between the tray icon launch and the boot effect but I wasn't familiar enough with the code base to diagnose it.

@vtavernier I checked in a fixe on the tray issue via PR #1199.
Would you mind testing it from you end too, please?

In regard of this PR (and I have to admit, I am new to hyperion effects, too),
my understanding is that the effect script might invoke a sequence of those "wrap" functions during execution.

If interpret it right, the intend of the isInterruptionRequested() call at every method, is stopping the effect and not continue with the sequence of activities in the effectg. The "interrupt effect" will e.g. be triggered when hyperion is stopped.

Will this PR do any harm that hyperion will not stop in a defined fashion or reasonable timeline, as the effect script might be a long running sequence? or what should be changed to avoid the highlighted invalid return type issue?

#1199 Looks good to me, with #1195 and without.

As for the isInterruptionRequested() I think it's more of a trade-off:

• Before Fix invalid return type when an effect has aborted #1195 changes: if the effect times out between the hyperion.abort() check and another method that returns "something", such as the imageWidth and imageHeight methods, the effect will crash as those will return None instead of an Int. It's unlikely but could technically happen.
• After Fix invalid return type when an effect has aborted #1195 changes: all the methods keep working as they do before the timeout, but it becomes the script's responsibility to check hyperion.abort() to abort for sure.

This depends on whether we want to allow the script to run some clean-up after the timeout expires, and prevent errors that could be confusing with the 1st option (since they usually report as "cannot multiply NoneType with Float" and aren't really clear), but I don't know what are the requirements for the effect behavior.

@vtavernier Thanks for sharing your view.

While thinking further on the topic in parallel, I came to the conclusion too, that doing things right outperform any performance driven optimizations. Given that the individual functions are small units of work, having them finished rightly might be better than pulling the plug the "hard way". So I am going to integrate #1195 into the latest PR and both of us do just some testing.... What do you think?

More concerning for me is that I saw that, if you run a background effect, it burns actively CPU cycles, even it is "hidden" by another priority in the source queue. So it does not sleep until it gets into focus again.

I also think it's not necessary to pull the plug the "hard way" as you put it. The effect module functions do a negligible amount of work: they would maybe run one time too many between the timeout and the next hyperion.abort check, which is basically nothing.

Letting the effects complete gracefully seems the most sensible option to me, and it does simplify the code (less checks for timeouts, no return type change, no need to document this effect in the Python module API, functions do behave as expected no matter the timeout).

As for the resources issue, on limited hardware, the background effects indeed put substantial strain on the CPU when enabled (on a Raspberry Pi Zero, the Rainbow Swirl consumes 20%CPU constantly). Maybe adding a hyperion.active method to notify the effect script if the current effect is active or not? I'm not familiar with all the effect scripts, but:

• Some scripts may have to run continuous updates to internal state, even if not visible
• This would allow skipping more resource-intensive methods that generate images, LED data, etc. which might be enough to significantly reduce the CPU load?

@vtavernier While removing the stratements...

In the various methods Py_RETURN_NONE is often returned to indicate a successful return rather than a nullptr as an indication for failure.
If you are saying that Py_RETURN_NONE is not appropriate, this stimulates the question at my end what is the appropriate alternative?

Sample:

PyObject* EffectModule::wrapImageDrawLine(PyObject *self, PyObject *args)
{
...
	if (argsOK)
	{
		QPainter * painter = getEffect()->_painter;
		QRect myQRect(startX, startY, endX, endY);
		QPen oldPen = painter->pen();
		QPen newPen(QColor(r,g,b,a));
		newPen.setWidth(thick);
		painter->setPen(newPen);
		painter->drawLine(startX, startY, endX, endY);
		painter->setPen(oldPen);

		Py_RETURN_NONE;
	}
	return nullptr;

I think it could be left that way. I only removed the "check timeout and return none" parts so the rest can be left as-is.

In the case of wrapImageDrawLine, there is indeed no need to return a particular value so it doesn't change anything. In the case of wrapImageWidth, an integer should always be returned, so returning None because the effect was aborted just creates more edge cases.

In the case of wrapImageDrawLine, there is indeed no need to return a particular value so it doesn't change anything. In the case of wrapImageWidth, an integer should always be returned, so returning None because the effect was aborted just creates more edge cases.

Thanks. I now get the point (which I might have missed before).

@vtavernier I just incorporated the changes of PR into #1199 to just having everything at one place for testing the PR.
I will close this PR for the time being.

Thank you again very much for your time on getting this fix in place!!!
And please continue joining us addressing further issues out there.

Looking forward seeing the next PR by yourself ;)