TriggerFx sometimes gets stuck on a note

Topics related to the API, programming discussions & questions, coding tips, bugs, etc. should go here.
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by Artcfox »

uze6666 wrote:Take two. Fixed and pushed. Let me know if all is fine.
All is not fine. The sounds are wrong now, but it's an easy fix—you actually made the exact same mistake that I did when trying to optimize things.

You can't move the:

Code: Select all

if(track->flags & TRACK_FLAGS_PLAYING){
from just above the:

Code: Select all

                        if(track->patchPlayingTime<0xff){
                                track->patchPlayingTime++;
                        }
call. Put that back where it was, and add its condition to the beginning of this if:

Code: Select all

if(track->patchCommandStreamPos!=NULL && ((track->flags & TRACK_FLAGS_HOLD_ENV)==0)){
Otherwise, when the:

Code: Select all

( (PatchCommand)pgm_read_word(&patchCommands[c1]) )(track,c2);
is invoked inside that block, and it ends up setting TRACK_FLAGS_PLAYING to false (like when it calls PatchCommand05), then the code below it needs to not run—which means that it needs to check that flag again.

Edit: Here is a diff of what I mean:

Code: Select all

diff --git a/kernel/uzeboxSoundEngine.c b/kernel/uzeboxSoundEngine.c
index b56e2af..5bddbf5 100644
--- a/kernel/uzeboxSoundEngine.c
+++ b/kernel/uzeboxSoundEngine.c
@@ -778,10 +778,9 @@ void ProcessMusic(void){
        for(unsigned char trackNo=0;trackNo<CHANNELS;trackNo++){
                track=&tracks[trackNo];
 
-               if(track->flags & TRACK_FLAGS_PLAYING){
 
                //process patch command stream
-                       if(track->patchCommandStreamPos!=NULL && ((track->flags & TRACK_FLAGS_HOLD_ENV)==0)){   //patchEnvelopeHold==false
+               if((track->flags & TRACK_FLAGS_PLAYING) && (track->patchCommandStreamPos!=NULL) && ((track->flags & TRACK_FLAGS_HOLD_ENV)==0)){ //patchEnvelop
 
                        //process all simultaneous events
                        while(track->patchCurrDeltaTime==track->patchNextDeltaTime){
@@ -808,6 +807,7 @@ void ProcessMusic(void){
                }
 
 
+               if(track->flags & TRACK_FLAGS_PLAYING){
                        if(track->patchPlayingTime<0xff){
                                track->patchPlayingTime++;
                        }
User avatar
uze6666
Site Admin
Posts: 4801
Joined: Tue Aug 12, 2008 9:13 pm
Location: Montreal, Canada
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by uze6666 »

Argh! Yes, you're right, fixed it. Though I think we should not have that (track->flags & TRACK_FLAGS_PLAYING) in the top if statement. If command 5 happens and the patch end command happens to be defined one or more frame later, it will prevent from reaching the patch end (0xff) code. Probably not that critical, but anyways. I may be wrong again though (!), got to get this out of the way I guess my brain doesn't want to see this code again!

Commited!
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by Artcfox »

uze6666 wrote:Argh! Yes, you're right, fixed it. Though I think we should not have that (track->flags & TRACK_FLAGS_PLAYING) in the top if statement. If command 5 happens and the patch end command happens to be defined one or more frame later, it will prevent from reaching the patch end (0xff) code. Probably not that critical, but anyways. I may be wrong again though (!), got to get this out of the way I guess my brain doesn't want to see this code again!

Commited!
Hmm, I hadn't considered that possibility. Now that the race condition with the vsync issue has been fixed, we no longer have to treat the symptom as we did before. I gave the code another once over, and I'm inclined to agree with you.

(I spent way more time than I'd like to admit looking for where the code checks for PATCH_END, because line 788 uses the magic constant 0xff directly, rather than the #define PATCH_END. Searching for PATCH_END brought me to PatchCommand14, which made me think that PatchCommand14 was the same as PATCH_END, but it is not. Grr.)
User avatar
uze6666
Site Admin
Posts: 4801
Joined: Tue Aug 12, 2008 9:13 pm
Location: Montreal, Canada
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by uze6666 »

(I spent way more time than I'd like to admit looking for where the code checks for PATCH_END, because line 788 uses the magic constant 0xff directly, rather than the #define PATCH_END. Searching for PATCH_END brought me to PatchCommand14, which made me think that PatchCommand14 was the same as PATCH_END, but it is not. Grr.)
I try as much as possible to not have magic number, but this one fell through. I replaced it with PATCH_END.
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by Artcfox »

I have some bad news. The original TriggerFx() glitch is not fixed. Luckily I was making a capture file at the time, so I can reproduce it.

We went through all the trouble of making sure that TRACK_FLAGS_PLAYING is unset before calling TriggerCommon, but you removed that condition from the if, and changed it to:

Code: Select all

if(track->patchCommandStreamPos!=NULL && ((track->flags & TRACK_FLAGS_HOLD_ENV)==0))
without ensuring that track->patchCommandStreamPos gets set to NULL before each call to TriggerCommon().

If you think that the track->patchCommandStreamPos != NULL is what the condition should test for, then we need to make sure that track->patchCommandStreamPos gets set to NULL before calling TriggerCommon (in case we are stealing a channel), and never gets set again until everything else in the track has been set up (is the last thing that happens inside TriggerCommon, and doesn't get reordered by the compiler).

I first verified that I can reproduce the sound glitch by playing back my .cap file, and then I added:

Code: Select all

track->patchCommandStreamPos = NULL;
before both calls to TriggerCommon(), recompiled, and indeed it no longer glitches.

The very last thing that TriggerCommon() does is set track->patchCommandStreamPos to what it reads from flash, so hopefully the compiler won't reorder that.

Here is the diff:

Code: Select all

diff --git a/kernel/uzeboxSoundEngine.c b/kernel/uzeboxSoundEngine.c
index e5816a9..ab8afca 100644
--- a/kernel/uzeboxSoundEngine.c
+++ b/kernel/uzeboxSoundEngine.c
@@ -1011,6 +1011,7 @@ void TriggerFx(unsigned char patch,unsigned char volume,bool retrig){
 
        Track* track=&tracks[channel];
        track->flags=TRACK_FLAGS_PRIORITY; //priority=1;
+        track->patchCommandStreamPos = NULL;
        TriggerCommon(track,patch,volume,80);
        track->flags|=TRACK_FLAGS_PLAYING;
 }
@@ -1034,6 +1035,7 @@ void TriggerNote(unsigned char channel,unsigned char patch,unsigned char note,un
                }else{
                
                        track->flags=0;//&=(~TRACK_FLAGS_PRIORITY);// priority=0;
+                        track->patchCommandStreamPos = NULL;
                        TriggerCommon(track,patch,volume,note);
                        track->flags|=TRACK_FLAGS_PLAYING;
                }
Alternatively, changing the condition to:

Code: Select all

if((track->flags & TRACK_FLAGS_PLAYING) && (track->patchCommandStreamPos!=NULL) && ((track->flags & TRACK_FLAGS_HOLD_ENV)==0))
also fixes the issue, but as you said in a previous reply, that doesn't allow PATCH_END to be reached, so I think that explicitly setting track->patchCommandStreamPos = NULL before calling TriggerCommon is the proper fix.
User avatar
uze6666
Site Admin
Posts: 4801
Joined: Tue Aug 12, 2008 9:13 pm
Location: Montreal, Canada
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by uze6666 »

Geee, will we ever fix this? :( I've put the track->patchCommandStreamPos = NULL; before triggerCommon. Commited.
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by Artcfox »

uze6666 wrote:Geee, will we ever fix this? :( I've put the track->patchCommandStreamPos = NULL; before triggerCommon. Commited.
I sure hope so!
User avatar
Artcfox
Posts: 1382
Joined: Thu Jun 04, 2015 5:35 pm
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by Artcfox »

This issue is still not fixed. :cry:

Again, I have a cap and hex file that can reproduce it, and changing the condition on line 782 of uzeboxSoundEngine.c to this:

Code: Select all

if((track->flags & TRACK_FLAGS_PLAYING) && (track->patchCommandStreamPos!=NULL) && ((track->flags & TRACK_FLAGS_HOLD_ENV)==0))
fixes it.
User avatar
D3thAdd3r
Posts: 3221
Joined: Wed Apr 29, 2009 10:00 am
Location: Minneapolis, United States

Re: TriggerFx sometimes gets stuck on a note

Post by D3thAdd3r »

That had to be one of the deepest seeded bugs I've heard of, and such a fundamental/critical part of the kernel no less. Great great work sir!

Wish that one squashed, any hope of a sneak peek on your latest developments in the near future? :mrgreen:
User avatar
uze6666
Site Admin
Posts: 4801
Joined: Tue Aug 12, 2008 9:13 pm
Location: Montreal, Canada
Contact:

Re: TriggerFx sometimes gets stuck on a note

Post by uze6666 »

Artcfox wrote:This issue is still not fixed. :cry:

Again, I have a cap and hex file that can reproduce it, and changing the condition on line 782 of uzeboxSoundEngine.c to this:

Code: Select all

if((track->flags & TRACK_FLAGS_PLAYING) && (track->patchCommandStreamPos!=NULL) && ((track->flags & TRACK_FLAGS_HOLD_ENV)==0))
fixes it.
Wait, I thought I already applied this fix! :shock: Ok, I've fix it straight in github. Hope it compiles since I can't test it now.
Post Reply