Fix #8885 (Unnecessary actions are performed by ExternalNotificationModule)

[polarikus]

Fix #8885

• I have tested that my proposed changes behave as described.
• I have tested that my proposed changes do not cause any obvious regressions on the following devices:
  • Heltec (Lora32) V3
  • LilyGo T-LoraPager

[thebentern]

The current (and desired) behavior is that any input broker event, not just long press, would dismiss an ext. notification. My preference would be to handle it in the input broker rather than PowerFSM, honestly, as it is a strange place dispatch an action for an input event vs the input broker. I would be in favor of removing the PowerFSM call to consolidate. @jp-bennett has thought as well I'm sure

[polarikus]

@thebentern

I didn't quite understand the concept.

Previously, there was a TODO in the powerFSM call section, which stated that we should NOT call powerFSM if it's a long press. I mixed this up - we should have done if (!INPUT_BROKER_IS_LONG_PRESS(event->inputEvent))

Previously, there was handling of externalNotificationModule->stopNow() here, which was also called again in the callback ExternalNotificationModule::handleInputEvent(const InputEvent *event)

I understand that you believe we should completely abandon:
#define EVENT_INPUT 17 in PowerFSM and wake devices from sleep using a different approach?

[mverch67]

Please check 2.7.16 (#8601) if it resolves your issue already.

[polarikus]

@mverch67
No, it didn't help. externalNotificationModule->stopNow() is still being called twice per event:

First in InputBroker::handleInputEvent

Second time in ExternalNotificationModule::handleInputEvent itself

I took the tag 2.7.16a597230 and added logs in these places:

int ExternalNotificationModule::handleInputEvent(const InputEvent *event)
{
    LOG_DEBUG("ExternalNotificationModule::handleInputEvent");
    if (nagCycleCutoff != UINT32_MAX) {
        stopNow();
        return 1;
    }
    return 0;
}

int InputBroker::handleInputEvent(const InputEvent *event)
{
    powerFSM.trigger(EVENT_INPUT); // todo: not every input should wake, like long hold release

    if (event && event->inputEvent != INPUT_BROKER_NONE && externalNotificationModule &&
        moduleConfig.external_notification.enabled && externalNotificationModule->nagging()) {
        LOG_DEBUG("InputBroker::handleInputEvent --> externalNotificationModule->stopNow()");
        externalNotificationModule->stopNow();
    }

    this->notifyObservers(event);
    return 0;
}

Log output:

DEBUG | 20:14:19 71 InputBroker::handleInputEvent --> externalNotificationModule->stopNow() //First call
INFO | 20:14:19 71 Turning off external notification:
INFO | 20:14:19 71 Stop RTTTL playback
INFO | 20:14:19 71 Stop audioThread playback
[ 71066][D][esp32-hal-cpu.c:244] setCpuFrequencyMhz(): PLL: 480 / 6 = 80 Mhz, APB: 80000000 Hz
INFO | 20:14:19 71 Turning off setExternalStates
INFO | 20:14:19 71 Input event 29! kb 0
DEBUG | 20:14:19 71 ExternalNotificationModule::handleInputEvent //Second call

I think we should abandon one of the places where we check for nagging and try to call stopNow. Most likely, it would be more correct to keep the handling in ExternalNotificationModule::handleInputEvent.

[mverch67]

From the logic in the code I think ExternalNotificationModule::handleInputEvent does nothing as the if (...) condition is not met. You could verify that if you place the LOG_DEBUG inside the if branch.

Edit: another question is if

    if (event && event->inputEvent != INPUT_BROKER_NONE && externalNotificationModule &&
        moduleConfig.external_notification.enabled && externalNotificationModule->nagging()) {
        LOG_DEBUG("InputBroker::handleInputEvent --> externalNotificationModule->stopNow()");
        externalNotificationModule->stopNow();
    }

is required at all as it seems that ExternalNotificationModule::handleInputEvent() is called anyway (via this->notifyObservers(event)) !?

In general I think the InputBroker should not know anything about any other modules and solely interface with the notify mechanism to decouple the components according the publisher/subscriber design pattern.

[polarikus]

Yes, that's exactly why I want to get rid of the stopNow() call in InputBroker. That was the goal of the PR.
It's just that next to it, I saw this TODO:

int InputBroker::handleInputEvent(const InputEvent *event)
{
    powerFSM.trigger(EVENT_INPUT); // todo: not every input should wake, like long hold release

And I decided to add a condition:

    if (!INPUT_BROKER_IS_LONG_PRESS(event->inputEvent)) {
        powerFSM.trigger(EVENT_INPUT);
    }

[thebentern]

However I also disagree with that TODO in the PowerFSM conceptually. I think the PowerFSM should focus on power related actions being dispatched only. It doesn't make sense for that event to interact with the ext. notification module directly

[mverch67]

Architecturally InputBroker should not know about PowerFSM. Instead, PowerFSM should subscribe(observe) input events if it should switch its fsm based on it and make its own decisions when to do it.

[polarikus]

@thebentern and @mverch67
Please take a look at whether the solution from the latest commit is suitable.
If not, we can split issue #8885 into two separate issues (removing PowerFSM from InputBroker as a separate request).

[mverch67]

@thebentern and @mverch67 Please take a look at whether the solution from the latest commit is suitable.

Yeah, now the separation of the two components is really clean, looks good except I would not expose

extern PowerFSMEventProcessor powerFSMEventProcessor;
extern CallbackObserver<PowerFSMEventProcessor, const InputEvent *> powerFsmInputObserver;

in the interface, these should be really local to (and hidden inside) PowerFSM.

[polarikus]

in the interface, these should be really local to (and hidden inside) PowerFSM.

Done