Advice regarding allowSomeUserActionsEvenWhenInCardRoutine

[RJ-Eckie]

Does anyone know the purpose of the mysterious allowSomeUserActionsEvenWhenInCardRoutine ? In audio_file_manager.cpp:

By default UI input is ignored while in sdRoutineLock but there is a focused effort in the code to have the vertical encoder still be interpreted during sdRoutineLock if allowSomeUserActionsEvenWhenInCardRoutine is true.

This makes it currently impossible to simplify the UI code by handling sdRoutineLock in a central place. I'd like to understand what exactly is happening here so I can create a clean implementation for this exception - or remove it if it's no longer used. Thanks 🙏🏻

[stellar-aria]

It allows processing of some UI rendering even while inside the card routine, since the card routine also does input event handling. See the views, such as

DelugeFirmware/src/deluge/gui/views/audio_clip_view.cpp

Lines 781 to 795 in be28b94

	ActionResult AudioClipView::verticalEncoderAction(int32_t offset, bool inCardRoutine) {
	if (!currentUIMode && Buttons::isShiftButtonPressed() && !Buttons::isButtonPressed(deluge::hid::button::Y_ENC)) {
	if (inCardRoutine && !allowSomeUserActionsEvenWhenInCardRoutine) {
	return ActionResult::REMIND_ME_OUTSIDE_CARD_ROUTINE; // Allow sometimes.
	}
	// Shift colour spectrum
	getCurrentAudioClip()->colourOffset += offset;
	uiNeedsRendering(this, 0xFFFFFFFF, 0);
	}
	return ActionResult::DEALT_WITH;
	}
	bool AudioClipView::setupScroll(uint32_t oldScroll) {

for an example.

The SD card routine is run on such a regular basis (and is apparently long enough cycle-wise) that not handling user input events during it would potentially impact responsiveness.

Solutions for this would be splitting up the larger monolithic sdCardRoutine into smaller tasks that can have input event handling done in between them, and maybe implementing a better event queue system. I'm not sure how we currently handle user events (whether queued or immediate).

[stellar-aria]

I would say try it, and if it doesn't have any horrendous obvious bugs we'll likely upstream it and try to handle any resulting issues architecturally, yes.

[seangoodvibes]

Where the code is triggered from isn't really my issue. I've currently rewritten the entire encoder-side of the ui and I have no issue calling it from the exact same position it's currently called from. (Replacing the community-tasks with mine.) Calling it from an interrupt sounds like a good idea - but that is separate from the issue I'm running into.

My problem is that I want to create a simple and central check against sdRoutineLock. Currently I'd say 98-99% of user input is not processed when sdRoutineLock is true. I would like to make that 100% by creating a central check and just not calling any of the interpretEncoder()/padAction()/buttonAction() (etc.) code at all during sdRoutineLock. We would need a total of two checks: one for handling presses (pads/buttons/encoders) and one for handling encoder rotations. This simplification is currently not possible because of the allowSomeUserActionsEvenWhenInCardRoutine exceptions. Currently there are almost a hundred checks for sdRoutineLock in the codebase.

So my concrete question is: seeing that allowSomeUserActionsEvenWhenInCardRoutine is used in only a few places, with no apparent consistency, and I would say in the current implementation with no real effect on responsiveness - can we just delete it? Removing these very specific and rare exceptions would allow for sdRoutineLock to just be a 100% lock for processing user input.

You can certainly try this.

But I'm not sure if centralizing that check and not calling the action functions while in the routine is the best solution.

I did that in automation view (by putting the check at the top of the button and pad action functions) because I thought that would simplify the logic

But upon benchmarking it, it was discovered that automation view's button and pad action functions were a lot slower than other button and pad actions while in the card routine.

The recommendation was to allow fast processor actions to happen while in the card routine and delay other slower actions from happening while in the card routine.

I believe Rohan has done some sort of exercise to determine what actions are fast and slow but this has not been reviewed since. This is evidenced by the in card routine check not being checked everywhere in the action functions.

Ultimately the objective here is to determine what actions when combined with being in the card routine would not slow down the deluge's performance as the deluge's performance is a combination of how long it takes to render and sd card streaming.

"You’ll get culling when rendering 128 samples is taking longer than outputting them or when the sd card can’t read the next cluster before it’s needed"

[RJ-Eckie]

The recommendation was to allow fast processor actions to happen while in the card routine and delay other slower actions from happening while in the card routine.

I can totally see that, but what you're describing is different from the results of allowSomeUserActionsEvenWhenInCardRoutine. (I'm just gonna call it allowSome for now.)

What you're describing is fully depends on sdRoutineLock:

• some code should be blocked during card routine
• some fast code should run during card routine

(Totally fair, and I'd probably personally create a centralised way of handling that as well - but for now I just want to focus on allowSome for a second.)

allowSome defines a different behavior:

• some code should always be blocked during card routine
  

  DelugeFirmware/src/deluge/gui/context_menu/midi_learn_mode.cpp

  Lines 85 to 87 in 92a67bf

  	if (sdRoutineLock) {
  	return ActionResult::REMIND_ME_OUTSIDE_CARD_ROUTINE;
  	}

• some code should be blocked during card routine except during AudioFileManager::loadAnyEnqueuedClusters() { loadCluster(cluster); }
  

  DelugeFirmware/src/deluge/gui/ui/keyboard/keyboard_screen.cpp

  Lines 650 to 652 in 92a67bf

  	if (inCardRoutine && !allowSomeUserActionsEvenWhenInCardRoutine) {
  	return ActionResult::REMIND_ME_OUTSIDE_CARD_ROUTINE; // Allow sometimes.
  	}

I'm just trying to work out that specific and rare behavior that allowSome enforces and my argument so far is that it just doesn't really do anything noticable. I'm keeping my branch 100% compatible with community so if we could agree on a consistent implementation of allowSome, or that it does so little that it can just be removed, that would make my life considerably easier.

For now I guess I'm just setting up a switchable test-case to turn it off and see whether anything breaks!

[nikodemus]

I was at some point considering changing those ifs to:

#define MAYBE_BLOCK(X) if (shouldBlock(X)) { return REMIND_ME_LATER; }

where X would be some kind of action-priority/heaviness enum. Would be also interesting to test the effect of adding static branch predictions either way to those.

[RJ-Eckie]

I was at some point considering changing those ifs to:

#define MAYBE_BLOCK(X) if (shouldBlock(X)) { return REMIND_ME_LATER; }

where X would be some kind of action-priority/heaviness enum. Would be also interesting to test the effect of adding static branch predictions either way to those.

I think it's useful to separate the two different discussions that are going on here:

1. How to deal with priority and/or code blocking during the SD routine is a very interesting and possibly complex discussion. The current setup to my understanding is as follows:

• The task manager manages proirity issues, dealing with hardware routines vs more high-level ui interactions

• During the SD routine, a bunch of code just doesn't get executed to prevent clashes. This isn't always implemented in the most consistent way, but it looks like that is the general design.

  I think implementing that system in a consistent way and evaluating the results of that would at this point be a more efficient way of spending effort than creating a new, more complex system - which we aren't sure we need without the evaluation of the current system. That's and interesting conversation to have

However I would like to separate that complex conversation from the way more simple question I'm asking here. Which is:

2. Does the current implementation of allowSomeUserActionsEvenWhenInCardRoutine do anything, at all? I'm arguing it doesn't.

   Agreeing on that answer would allow us to remove a range of checks that create a significant amount of chaos in UI code as it is currently, without making any changes to the priority and blocking behavior. The conversation about fundamental changes to the priority and blocking implementation would be a separate one