Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New method for handling custom alarms #961

Open
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

eried
Copy link
Contributor

@eried eried commented Feb 16, 2020

This fixes #960

Available settings in src/music.c:
USE_NEW_CUSTOM_ALARM_MODE, if is 0 then the behaviour is exactly the same as the original

CUSTOM_ALARM_IGNORE_MS, is the time in ms that the sound will not be triggered. So if is 200 ms and the user switch from A to C, sound B wont be played if the user took less than 200 ms on moving the switch from B to C.

@eried eried requested a review from TheRealMoeder February 16, 2020 23:36
Copy link
Contributor

@TheRealMoeder TheRealMoeder left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the general idea, but I'd suggest working this into the config (tx.ini?) files instead of the define and also add menu options for setting this up? Maybe we can have the "old" behavior when the value for CUSTOM_ALARM_IGNORE_MS is "0"?

Edit:
Another thing comes to my mind: Maybe ignoring all custom alarms coming in fast isn't desirable: Imaging flipping a switch immediately before a telemetry alarm with custom sound comes in. So maybe this behavior should be exclusive for switch custom alarms? I'm open for discussion on this matter.

@eried
Copy link
Contributor Author

eried commented Feb 17, 2020

I like the general idea, but I'd suggest working this into the config (tx.ini?) files instead of the define and also add menu options for setting this up? Maybe we can have the "old" behavior when the value for CUSTOM_ALARM_IGNORE_MS is "0"?

I agree that a menu entry would be ideal, I am going to check that section on the source. I left that as a define thinking that in the unlikely case that someone might want that old behaviour, it still could be engaged.

Edit:
Another thing comes to my mind: Maybe ignoring all custom alarms coming in fast isn't desirable: Imaging flipping a switch immediately before a telemetry alarm with custom sound comes in. So maybe this behavior should be exclusive for switch custom alarms? I'm open for discussion on this matter.

You are right, I was not aware or explored those telemetry alarms and others. In this case it would require a "source" flag (i.e. priority and have that as a setting in the menu maybe?).

I am thinking a solution could be:

  1. Add priority to the int AUDIO_AddQueue(u16 music) { function
  2. Expose that priority in options:
    Priority for switch voice: high|normal

In that way the "buzzer" functionality (that was what I wanted to fix later) could use those rules.

I was just blinded for how cool it feels that the sounds are skipabble. https://youtu.be/Bhd3Ko32Q8U?t=264

@TheRealMoeder
Copy link
Contributor

I agree that a menu entry would be ideal, I am going to check that section on the source. I left that as a define thinking that in the unlikely case that someone might want that old behaviour, it still could be engaged.

Menu is pretty simple; I can add that later in case you don't want to dig into the source too much.

You are right, I was not aware or explored those telemetry alarms and others. In this case it would require a "source" flag (i.e. priority and have that as a setting in the menu maybe?).
I am thinking a solution could be:

  1. Add priority to the int AUDIO_AddQueue(u16 music) { function
  2. Expose that priority in options:
    Priority for switch voice: high|normal

I think this really only applies to switches, so we might as well make a seperate case for switches. How about somehow passing a switch id (or group) together with the voiceid for the queue voice to always replace queue entries for the same switch id/group so we only have the latest switch state in the queue? Will probably require a seperate AUDIO_AddQueueSwitch (u16 music u16 group) or similar. The reason behind this is, you could also be flipping to switches at the same time (not that uncommon in the heat of the flight) and wouldn't want to lose any of the announcements of either switch.

While at reworking, I think it also makes sense for AUDIO_AddQueue to check, whether the music being added to the queue is the same as the last entry in the queue, and in that case discard the duplicate entry.

In that way the "buzzer" functionality (that was what I wanted to fix later) could use those rules.

I was just blinded for how cool it feels that the sounds are skipabble. https://youtu.be/Bhd3Ko32Q8U?t=264

I wasn't aware that someone finally found a fix for the "saving" issue - great!

@goebish you always have some good ideas, any comments on this issue?

@eried
Copy link
Contributor Author

eried commented Feb 21, 2020

Menu is pretty simple; I can add that later in case you don't want to dig into the source too much.

That would be awesome.

While at reworking, I think it also makes sense for AUDIO_AddQueue to check, whether the music being added to the queue is the same as the last entry in the queue, and in that case discard the duplicate entry.

Not only duplicates, but also how many ms since it was triggered, to skip the sounds while switching a 3 state switch. I do not really agree with keeping ALL the switches voices all the time, I like how it drops or interrupts voices. But ofc this would need some kind of survey, or option.

I wasn't aware that someone finally found a fix for the "saving" issue - great!

Yes, sadly is a hardware issue that will need a pcb revision.

@TheRealMoeder
Copy link
Contributor

While at reworking, I think it also makes sense for AUDIO_AddQueue to check, whether the music being added to the queue is the same as the last entry in the queue, and in that case discard the duplicate entry.

Not only duplicates, but also how many ms since it was triggered, to skip the sounds while switching a 3 state switch. I do not really agree with keeping ALL the switches voices all the time, I like how it drops or interrupts voices. But ofc this would need some kind of survey, or option.

Well, the switch issue would be solved with adding the alarm group to a queue entry. Which means an alarm from the same group will cancel old alarms in the queue with the same group id. At least that's how thought of it. Do you get what I mean?

@eried
Copy link
Contributor Author

eried commented Feb 23, 2020

Yes I understand about the "groups", that concept makes skipping voices harder to implement, and it just solves the "switching from A to C without hearing B".

Just for the sake of it, I am going to list the cases:

  1. Able to skip currently playing voices:
    video

  2. Except if is an alarm or some important stuff:
    video

Those two are common sense, and not open for discussion, right?
3) Skip voices from the same switch:
video

This is also common sense. However:
4) Skip pending voices, even if they are not from the same switch:
video

In my opinion, in that example the user should not have to hear all the switches positions in a queue. That could be the setting of "priority of switch position", however I still do not see much of a point of enqueueing (and hearing) all voice switches.

@TheRealMoeder
Copy link
Contributor

  1. Able to skip currently playing voices:
  2. Except if is an alarm or some important stuff:
    Those two are common sense, and not open for discussion, right?

Right, would be solvable with the switch group concept.

  1. Skip voices from the same switch:

This was what started the whole discussion, so nothing to add here.

  1. Skip pending voices, even if they are not from the same switch:
    In my opinion, in that example the user should not have to hear all the switches positions in a queue. That could be the setting of "priority of switch position", however I still do not see much of a point of enqueueing (and hearing) all voice switches.

This is the only case which would need the priority setting. But I'm having some issues about making setting each alarms priority a requirement. From a user perspective, I think telemetry, timer and battery alarms are all somehow high priority unless specifically changing this behavior, so this will probably be the default behavior. Your wish number 4 could be achievable by having the option to "upgrade" specific switch settings to the same high priority level as the ones above.

I do not think we can omit enqueuing all switches, as this will make the behavior somewhat unpredictable and I again think of the case of dual switch flips at the same time.

@eried
Copy link
Contributor Author

eried commented Feb 27, 2020

I am still thinking on a solution, so question for you @TheRealMoeder, Imagine 2 switches, you switch one, voice start playing... switch the 2nd one. Does the first voice gets interrupted?

@TheRealMoeder
Copy link
Contributor

I am still thinking on a solution, so question for you @TheRealMoeder, Imagine 2 switches, you switch one, voice start playing... switch the 2nd one. Does the first voice gets interrupted?

I can imagine both behaviors, but I tend to prefer the first voice not being interrupted

if (CHAN_ButtonIsPressed(button, BUT_EXIT)) {
PAGE_Pop();
if (flags & BUTTON_RELEASE)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lint Error:

101:  Line ends in whitespace.  Consider deleting these extra spaces.  

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

CUSTOM SOUNDS should be skippable, and not every sound should go to the queue
3 participants