Button release events not firing correctly about proffieos

[profezzorn]

Which event are you actually referring to? Buttons now have lots of associated events now, and some of them (the SAVED ones) are expected to behave kind of like this to distinguish clicks from double/triple/quadruple clicks.

[SA-22C]

Here are two examples, centering around HELD events:

EVENT: Power-Held#2 ON millis=80079
unit = 1 vol = 0.50, Playing TROS_Graflex_Proffie/bgnlb/bgnlb2.wav
channels: 1 rate: 44100 bits: 16
Playing TROS_Graflex_Proffie/swingl/swingl01.wav
channels: 1 rate: 44100 bits: 16
Playing TROS_Graflex_Proffie/lb/lb.wav
channels: 1 rate: 44100 bits: 16
Audio underflows: 1

So right here, I released the button, but no Power-Released event was emitted. So I pressed and released the Power button and as you can see below:

EVENT: Power-Pressed#1 ON millis=82308
EVENT: Power-Pressed ON millis=82308
EVENT: Power-Released#1 ON millis=82366
EVENT: Power-Released ON millis=82366
unit = 4 vol = 0.50, Playing TROS_Graflex_Proffie/endlb/endlb3.wav

The endlb wav only plays on the Pressed/Released combo, because the 'released' event is not emitted on HELD.

Same behaviour on HELD using Aux:

EVENT: Aux-Pressed#1 ON millis=109712
EVENT: Aux-Pressed ON millis=109712
EVENT: Aux-Held#1 ON millis=110013
unit = 1 vol = 0.50, Playing TROS_Graflex_Proffie/bgnlock/bgnlock4.wav
channels: 1 rate: 44100 bits: 16
Playing TROS_Graflex_Proffie/lock/lock01.wav
channels: 1 rate: 44100 bits: 16

The Aux button is released here, but again no Released event was emitted. So like above, a short press of Aux triggered the 'Released' event and the endlock wav plays

EVENT: Aux-Pressed#1 ON millis=112516
EVENT: Aux-Pressed ON millis=112517
EVENT: Aux-Released#1 ON millis=112693
EVENT: Aux-Released ON millis=112693
unit = 4 vol = 0.50, Playing TROS_Graflex_Proffie/endlock/endlock2.wav

[profezzorn]

Hmm, I think maybe I know what's going on here. Although I'm not sure what we should do about it.
Right now, when you return true in an action, the "current_modifier" is cleaned out, to signal that you have claimed this action.
When that happens, there is no release event. The intent here is that when you have AUX+POWER do one thing, and AUX-RELEASE do another, then you don't want both events, just one of them.

However, I'm not sure that this should apply to RELEASE events, maybe it should only apply to CLICK events?
The problem is of course that HELD and RELEASE events are meant to be used together, but right now, if the HELD action returns true, then the RELEASE event will not be generated at all... One workaround would be to have the HELD action return false, but I'm not sure that's a good solution.

[SA-22C]

I'm not entirely sure what the right solution is, as I don't fully understand what new functionality you've introduced. However, I think that a release event should always occur when a button is released, because that's only logical. You cannot have a press without a release, no matter what functions you're hoping to achieve with multiple button combinations. Perhaps you're right when you state that CLICK events that occur as a combo could suppress an individual RELEASE event and instead emit a combination RELEASE (ie: AUX + POW emits an AUX_POW_RELEASE and POW+AUX would emit a POW_AUX_RELEASE) That way you avoid conflicts when performing combos but still follow the logical chain that a button press must also have a release.

[artandtechvof]

Not sure if its related, but I had the same happening for a 1 button setup after lockup was started. When the power button was released lockup kept going. only by short pressing it again, was I able to end the lockup.
After some figuring out how stuff works, my solution to the problem was to have my prop-file return 'false' after the event was handled;

			case EVENTID(BUTTON_NONE, EVENT_CLASH, MODE_ON | BUTTON_POWER):
				if (!SaberBase::Lockup()) {
				  if (pointing_down_) {
					SaberBase::SetLockup(SaberBase::LOCKUP_DRAG);
				  } else {
					SaberBase::SetLockup(SaberBase::LOCKUP_NORMAL);
				  }
				  SaberBase::DoBeginLockup();
				  return false;//to keep track off button released      <<<<---------------
				}
				break;

In most of the cases, the case statement returns true, but if you set it to false, the release event will be send, and lockup ends on release of the button in my case.

[profezzorn]

I think this should be fixed now.

[artandtechvof]

Unfortunately, the problem still exists in ProffieOS 5.x master branch.
Using all stock settings, going into lockup (or melt) will keep on going when button is released.
Only after additional short click melt or lock will be released.
For me returning 'false' (in stead of true) from the lock and melt switch-cases in saber.h resolves this problem.

[profezzorn]

Is this a plain 5.x, or a fork with your changes in it?
Does it have this line in it?

ProffieOS/props/prop_base.h

Line 1369 in 70c213d

current_modifiers &= button;

[artandtechvof]

Yes indeed, this was a clean and fresh fetch from your master, and the prop-file has then no alteration and straight upload to a ProffieV2.2

if (Event2(button, event, current_modifiers | (IsOn() ? MODE_ON : MODE_OFF))) {
  current_modifiers &= button;
  return true;
}

(maybe re-open this issue?)

[profezzorn]

Which switch cases did you change to return true exactly?
I'm wondering if that is in fact the right thing to do at this point...

[artandtechvof]

In saber.h;

Change line 136 and 150 (lockup and melt) to retrurn false, and everything works for me;
If these functions return true, the event is handled, and further button presses will not trigger any event. When they return false, the event is not handled and the button event keep on sending events after lockup and melt have been started. Could that be about right?

ProffieOS/props/saber.h

Lines 126 to 138 in 70c213d

	// Lockup
	case EVENTID(BUTTON_NONE, EVENT_CLASH, MODE_ON | BUTTON_POWER):
	case EVENTID(BUTTON_NONE, EVENT_CLASH, MODE_ON | BUTTON_AUX):
	if (!SaberBase::Lockup()) {
	if (pointing_down_) {
	SaberBase::SetLockup(SaberBase::LOCKUP_DRAG);
	} else {
	SaberBase::SetLockup(SaberBase::LOCKUP_NORMAL);
	}
	SaberBase::DoBeginLockup();
	return true;
	}
	break;

and

ProffieOS/props/saber.h

Lines 145 to 152 in 70c213d

	case EVENTID(BUTTON_NONE, EVENT_STAB, MODE_ON | BUTTON_POWER):
	case EVENTID(BUTTON_NONE, EVENT_STAB, MODE_ON | BUTTON_AUX):
	if (!SaberBase::Lockup()) {
	SaberBase::SetLockup(SaberBase::LOCKUP_MELT);
	SaberBase::DoBeginLockup();
	return true;
	}
	break;

[profezzorn]

I will change these to return false.
Are there similar events in other prop files that will need to be updated as well?

[artandtechvof]

As far as I can see, it is primarily for all events that rely on the EVENT_RELEASED ending some action.
For now that are only the different kind of Lockups that we have. So yes, each of the prop-files that come with ProffieOS (saber_fett263_buttons.h, saber_sa22c_buttons.h, saber_shtok_buttons.h) will need to alter the return value of the cases that have the SaberBase::DoBeginLockup();
(I can make an overview of all propfiles and functions/linenumbers for you tomorrow, for now its getting sleeping time here in The Netherlands.)

[profezzorn]

As I think about this some more, there are basically two ways to go.

Let's consider two use cases and how they could be implemented:

A) "next preset" (hold power + clash while off). in this case, we want to make sure to not trigger whatever action power normally triggers.
B) "lockup" (hold power + clash while on) in this case, we still want the release event for the power button so that we can end lockup.

The prop is the only file that knows the difference between (A) and (B), so it must do something different for those two cases.

Option #1: RELEASE = SHORT_CLICK | LONG_CLICK

With this interpretation, having an event triggered by RELEASE is basically the same as having two case statements, one for short click and one for long click. Since in the (A) case, no click should be generated, no RELEASE event should be generated either. Thus, the prop must return false in the (B) case to keep getting the release and/or click events.

Option #2: Create a new event EVENT_CLICK which is equal to EVENT_CLICK_SHORT | EVENT_CLICK_LONG

This new event would only be emitted if nothing is triggered when we send the EVENT_CLICK_SHORT or EVENT_CLICK_LONG.
With this new event, EVENT_RELASE doesn't have to be considered a "click", and could be sent unconditionally.
In the prop file, would need to be careful to use EVENT_CLICK in the (A) use case and EVENT_RELASE in the (B) use case.

While option #2 has some advantages, and may be more backwards compatible, it seems more logical to have the first event decide if the action is "done" or not rather than having the second event decide it.... (If that makes sense...)
Although, I suppose, with option #2, the first event can still return false to keep everything going...

[profezzorn]

Option #3: Same as Option #2 but don't make a new event.
If people want to trigger something on both EVENT_CLICK_SHORT and EVENT_CLICK_LONG, they can always use two case statements...

[profezzorn]

Option #3 implemented.

[artandtechvof]

Although I do like the reasoning behind #1, that you have to be aware in your prop file, if an event should be marked as done or still in progress (return true; =done.. , return false; =waiting for additional events for this action) It is not very backwards compatible, and requires updating prop files to this new standard.

#3 seems reasonable;
Just did a fresh fetch of the current master, and after upload, it is working correctly now for all Lockup event types

Thx!