i2c multiplexer for oled's

giuliomoro

you are specifically meant to run this project as a separate project from your Bela code. As this project contains a main function, it will never call Bela's setup, render, etc. If you want to use this code as part of a C++ project including audio processing code using the Bela API, then you should rename the main function to something else and call it from within your setup() function.

nickcamillo I got around these errors by commenting out both lines

that's now on the main branch with a few more edits.

nickcamillo I'm trying to run the update script to pull in the u8g2 dependency in a different project but I keep getting the warning about usage that displays if no directory is supplied. Am I using it correctly?

this needs to run so that the machine running update.sh can access the specified path. For instance, if you have /path/to/u8g2/on/local/machine on your local computer and run update.sh on the board, it won't work. I just updated to the latest u8g2 (1bd25a7e657e20ef0efdab1c9cb5ba4e0aeae75b) and updated the update script so that it should be mostly automated. This update and updated update can be found on the update branch.

nickcamillo

giuliomoro thank you, before I try this I just want to clarify what you're saying.

If you want to use this code as part of a C++ project including audio processing code using the Bela API, then you should rename the main function to something else and call it from within your setup() function.

What you're suggesting here would mean that I'm no longer running a background service right? I think I need to run the OLEDs as a background service because I get dropped audio frames when I try to update the screens directly via the C++ api from within my render.cpp during audio playback.

you are specifically meant to run this project as a separate project from your Bela code. As this project contains a main function

Do you mean that this example project was meant to be run that way, or that implementing the background service works that way? Like I should build and run that separate project as a service and then switch back to my main project and run my normal project on the bela?

giuliomoro

You can build the O2O project as it ships from the Bela IDE and even run it from within it, but it won't process any audio. This is C++, so the program will start execution from the main() function. In most Bela project, there is no main() function, so the default one is used under the hood, which in turn calls setup(), render(), cleanup(). In this project, there is a main() function and so the default one won't be called and setup(), render() and cleanup() won't run.

If you want a single program to run both the OLED(and optionally OSC) stuff AND the audio, then you wouldn't run two different processes and you would simply set the one program to run on boot from the IDE. If you previously enabled running the program as a separate service, you should disable that.

nickcamillo I think I need to run the OLEDs as a background service because I get dropped audio frames when I try to update the screens directly via the C++ api from within my render.cpp during audio playback.

This is not because you are running them from within the same process, rather because you are doing something from the audio thread that takes too long and/or are running non real-time-safe code from within the audio thread. You should reduce computation in the audio thread and push any long or non real-time-safe computations to a different thread. If you were to share your code I could probably help with that.

nickcamillo

giuliomoro Thanks for the explanation. The code in question is very simple but I accidentally deleted it during this process. I'm trying to animate a playhead moving across a waveform on the OLED during playback. It worked great, just dropped frames. Every time readPointer % playheadInterval == 0 where playheadInterval = gNumberOfFrames / displayWidth i'm calling a drawWaveform function which first calls switchTarget to the correct display, clears the display's buffer and then draws a waveform and the playhead. Depending on the length of the file, this call happens more often than others but I optimized when it gets called so that it only happens once per pixel.

so basically something like this

...
void render(BelaContext * context, void * userData) {
  for (unsigned int n = 0; n < context -> audioFrames; ++n) {
  
    if (readPointer % playheadInterval == 0) {
      drawWaveform(playheadIndex);
    }
  }
}

Would love to see how to handle this in another thread, and if you know how to only update certain parts of an individual display using u8g2 I'd love to hear that too haha.

giuliomoro

hmmm what is drawWaveform()? If that is computing all the pixels of the screen, it could be already very time consuming, possibly enough to cause a dropout. If in addition to that it also calls sendBuffer() then it really shouldn't be called from there because that operation takes several milliseconds to complete. Keep in mind that the display has a limited refresh rate, this depends on how many pixels you redraw (normally all of them)* and the I2C speed (which should already be at the maximum). This means that you should only try to draw it every so often, e.g.: every 50 ms. This is best done from a separate thread, where you have something like:

void writeToScreen(void*)
{
    while(!Bela_stopRequested()){
        // set all the pixels
        // .... your code here ...
        // send the data via I2C to the display
        u8g2.sendBuffer(); // this will take time to complete, possibly several milliseconds
        // sleep a bit if needed to lower the frame rate:
        // the overall period of the loop will be primarily determined by this sleep + the time it takes to complete sendBuffer() above.
        usleep(10000);
    }
}

you'd start this thread from setup() with something like:

Bela_runAuxiliaryTask(writeToScreen);

now, the magic is all in ... your code here ... there you'll probably put something very similar to drawWaveform(playheadIndex). You need playheadIndex to be a global variable so that it can be written from the audio render() thread and read from the writeToScreen() thread. This way you don't even need the if(readPointer % playheadInterval == 0) logic in render().
You can remove the usleep() line if you want the highest refresh rate that the I2C bus will allow, but that may put a significant load on your CPU and may make the IDE unresponsive, but it should in no way cause audio dropouts. If the IDE becomes unresponsive, briefly press the button on the Bela cape to stop the execution of the Bela program and wait a few seconds for the IDE to come back to life.

nickcamillo and if you know how to only update certain parts of an individual display using u8g2 I'd love to hear that too haha.

not sure. In csrc/u8g2_buffer.c , u8g2_send_buffer(), which is called from U8G2::sendBuffer(), calls u8g2_send_tile_row() and ultimately u8x8_DrawTile(). This last one u8x8_DrawTile() is also available directly from U8G2::drawTile(uint8_t x, uint8_t y, uint8_t cnt, uint8_t *tile_ptr). Not sure about the semantics of the arguments, but you can try looking at the existing code. Note that some displays will need to call refreshDisplay() for any change applied with drawTile() to take place. A tile seems to be an 8x8 pixel square, according to grepping through the docs.

nickcamillo

Thanks so much for all these explanations! I really appreciate all your help! I imagine this will all be very helpful to the next noobie who decides to try this haha. About to dive in to give this a shot and I have some more questions.

I'm noticing that the loop function in the Trill example (that I've already copy-pasta'd into my render.cpp) is also making use of that pattern with its loop function. Since loop is already calling usleep and being run during !Bela_stopRequested() does that mean there will be a collision? Do I need to combine these two auxiliary tasks somehow?

Also, it's only now occurring to me (after reading your response like 10 times and seeing how this pattern is used by Trill) that this function is being run automatically during every Bela stop request as opposed to when I know there is an update that needs to be made to the screen. Before I was only calling drawWaveform when I knew there was an update that had to be made. But now it seems like the screen(s) will be sent new buffers at regular intervals, correct?

I have 5 displays, and one or two of them is likely to be needed to be updated at a time depending on what mode the system is in. If we're regularly interrupting to perform this screen update instead of manually calling a draw function on a per-display basis when an update needs to be made, does it make more sense to think of writeToScreen as writeToScreens in which we loop through each screen and update? Does it make sense to have some diffing logic in the ...your code here... section that skips to the next screen if no diff exists for that screen and ultimately skip to the usleep() function if it's determined that no updates need to be made to any screens?

giuliomoro

nickcamillo Since loop is already calling usleep and being run during !Bela_stopRequested() does that mean there will be a collision? Do I need to combine these two auxiliary tasks somehow?

No there won't be any problem. You can have several threads at once, they'll all take their share of CPU time when needed: when a thread calls usleep() the cpu becomes available for other threads to run.

Also, it's only now occurring to me (after reading your response like 10 times and seeing how this pattern is used by Trill) that this function is being run automatically during every Bela stop request as opposed to when I know there is an update that needs to be made to the screen.

That's not how it works. Bela_stopRequested() is a function that returns immediately. It returns true only when the program has been requested to stop (e.g.: when you press the stop button in the IDE or when you press the button on the Bela cape) , otherwise it returns false.
So the construct

while(!Bela_stopRequested) {
// Do stuff
// Sleep a bit
}

Means that "stuff" is done periodically as long as the program should keep running. Note the ! before Bela_stopRequested(): the expression in parenthesis will be true while the program is running regularly and it will become false when the program is stopping. When the program is stopping and the condition becomes false, the loop will no longer be executed and the function will return.

Before I was only calling drawWaveform when I knew there was an update that had to be made. But now it seems like the screen(s) will be sent new buffers at regular intervals, correct?

That's correct. If you want to send only when there is an update ready, that will help you save some cpu and it's going to free up time on the I2c bus for the other screens so you can achieve higher throughpit overall. To that effect you can use an additional global flag, for instance one that gets set by the audio thread when new data is to be written and that gets reset by this thread when data is actually being written, or keep a local copy of the relevant global variable in this thread and only update the screen if the globsl value has changed with respect to the local copy.

As you are using 5 screens and they are all sharing the same I2C bus, it is convenient to have all the writes happen from the same thread, so in the while loop you'd check whether each screen has to be updated and if it does, update it, then move on to the next one and so on. You can make the usleep conditional: if one or more screens were updated, do not sleep, otherwise sleep.

nickcamillo

Gotcha, this all makes total sense now. Seems to be working for me! You're seriously the man. Can I buy you a beer 🍺 ? Haha. I'm sure I'll be begging for your help many more times in the next few weeks and months but this has been a tremendous help and I really really appreciate it.

nickcamillo

@giuliomoro I've got a button that only on a debounced press and a debounced release sets gHasDisplayUpdatedForModeChange = 0 which I then set to 1 from the aux task after updating the display. I've been noticing some glitchy behavior in the OLED rendering and started debugging to figure out what could be going wrong.

in the aux task, I set up a conditional print:

if (gHasDisplayUpdatedForModeChange == 0) {
  rt_printf("MODE CHANGED\n");
}

and I'm noticing that it's only firing sometimes. I have a log on button press/release that prints 100% of the time when that global variable is set to 0, but the log in the aux task only fires maybe 1/3 of the time, even though the aux task is definitely running. Any idea what could be going wrong here?

giuliomoro

nickcamillo but the log in the aux task only fires maybe 1/3 of the time, even though the aux task is definitely running. Any idea what could be going wrong here?

Who is resetting the global variable to 1? The customary way would be:

if (gHasDisplayUpdatedForModeChange == 0) {
    rt_printf("MODE CHANGED\n");
    /// TODO: code goes here to update the display
    gHasDisplayUpdatedForModeChange = 1;
}

if you are resetting it from a different thread than the one who is reading it, then it could be that it gets reset before it is read from that thread.

Another thing to keep in mind is that if you are setting the flag to 0 in rapid succession and the thread reading the flag and sending to the display doesn't have time to run in-between the flag being set, then it's expected that you'd get more prints from the thread setting it than from the thread reading it. From your description it shouldn't be the case here, because the physical gesture of pressing the button should be slow enough that the display thread should always have the time to run in between button presses. However, if your debouncing is not working properly, you could have the audio thread setting the flag multiple times in a single button press (and possibly within a single execution of render()) and then you'd see the other thread only printing once.

As usual, seeing a bit more of your code would help understanding the issue.

nickcamillo

giuliomoro

if (gHasDisplayUpdatedForModeChange == 0) {
rt_printf("MODE CHANGED\n");
// u8g2 clear buffer / draw / send buffer
gHasDisplayUpdatedForModeChange = 1;
}

That's exactly how it works. The variable read and reset happen in the same thread. I'm noticing this issue before anything complex happens in the drawing at the very top of the aux function.

I know it's not a debounce issue because I'm logging Button 1 Pressed and Button 1 Released only once per debounced event, which is working properly:

void handleButtonLeft1Pressed() {
    rt_printf("Button Left 1 pressed\n");

    gHasDisplayUpdatedForModeChange = 0;
}

void handleButtonLeft1Released() {
    rt_printf("Button Left 1 released\n");
    
    gHasDisplayUpdatedForModeChange = 0;
}

These functions get called by a debounce function that I took from the C++ tutorials that gets called within render during every digital frame along with a whole bunch of other buttons. If you know of a more programmatic way to handle this, I'd love to see it. I'm currently duplicating that debounce function for every button.

void debounceButtonLeft1(int status) {
    if (gButtonLeft1DebounceState == kButtonOpen) {
        // Button is not pressed, could be pressed anytime
        // Input: look for switch closure

        if (status == 0) {
            gButtonLeft1DebounceState = kButtonJustClosed;
            gButtonLeft1DebounceCounter = 0;

            handleButtonLeft1Pressed();
        }
    } else if (gButtonLeft1DebounceState == kButtonJustClosed) {
        // Button was just pressed, wait for debounce
        // Input: run counter, wait for timeout

        if (++gButtonLeft1DebounceCounter >= gButtonDebounceInterval) {
            gButtonLeft1DebounceState = kButtonClosed;
        }
    } else if (gButtonLeft1DebounceState == kButtonClosed) {
        // Button is pressed, could be released anytime
        // Input: look for switch opening

        if (status != 0) {
            // Found switch opening (do nothing but change state)
            gButtonLeft1DebounceState = kButtonJustOpened;
            gButtonLeft1DebounceCounter = 0;
        }
    } else if (gButtonLeft1DebounceState == kButtonJustOpened) {
        // Button was just released, wait for debounce
        // Input: run counter, wait for timeout

        if (++gButtonLeft1DebounceCounter >= gButtonDebounceInterval) {
            // Timeout: now we can start waiting for switch to open	
            handleButtonLeft1Released();
            gButtonLeft1DebounceState = kButtonOpen;
        }
    }
}

void render(BelaContext * context, void * userData) {
    for (unsigned int n = 0; n < context -> digitalFrames; n++) {
        debounceButtonLeft1(buttonLeft1Status);
    }
}

What's even weirder about this is that the displays do seem to be updating properly and consistently on these mode changes, which is only possible if that logic around the print statement is true. They just don't seem to be printing.

giuliomoro

nickcamillo That's exactly how it works. The variable read and reset happen in the same thread. I'm noticing this issue before anything complex happens in the drawing at the very top of the aux function.

can you post the full code of the thread please?

Wondering if it's an artifact of using rt_printf() from different threads... try using printf() from the auxiliary thread, however that may reorder the outputs and you could see first the output of printf() and then that of rt_printf() if the latter was called only a handful of milliseconds before the former.

nickcamillo

I discovered what was causing the problem while trying to strip the code down to the minimum to reproduce. Posting for posterity.

void updateDisplays(void *) {
    while (!Bela_stopRequested()) {
        if (gDisplaysInitialized == 1) {
        	if (gHasDisplayUpdatedForModeChange == 0) {
        		printf("MODE CHANGED\n");
        	}

			bool shouldPause = gHasDisplayUpdatedForModeChange;

			if (gHasDisplayUpdatedForModeChange == 0) {
				if (gMode == kMode1) {
					if (!gHasDisplayUpdatedForModeChange) {
						rt_printf("drawing mode 1\n");

						gHasDisplayUpdatedForModeChange = 1;
					}
				} else if (gMode == kMode2) {
					if (!gHasDisplayUpdatedForModeChange) {
						rt_printf("drawing mode 32\n");

						gHasDisplayUpdatedForModeChange = 1;                                       
					}
				}
			}
        }

		if (shouldPause) {
			usleep(gTaskSleepTime);
		}
	}
}

I was conditionally running the usleep to try and eek out a bit faster refresh rate like you suggested, but it seems like that was causing the issue. Setting it to always pause seems to have resolved it and I'm not really noticing a rate difference.

giuliomoro

Where is the data sent to the display in your code? I would really appreciate if you were to post your full code, or at least all the relevant parts instead of having to second-guess things ... Not because I am curious, but because it is important in order to understand the issue at hand.

First step simplification: your code has a lot of redundancy, it is checking for the value of gHasDisplayUpdatedForModeChange so many times ... and it could be simplified significantly, e.g."

void updateDisplays(void *) {
	while (!Bela_stopRequested()) {
		// early condition check, easier to read for me
		if (!gDisplaysInitialized || gHasDisplayUpdatedForModeChange)
		{
			usleep(gTaskSleepTime);
			continue;
		}
		// if we are here, we need to send a refresh
       		printf("MODE CHANGED\n");
		if (gMode == kMode1) {
				rt_printf("drawing mode 1\n");
		} else if (gMode == kMode2) {
				rt_printf("drawing mode 32\n");
		}
		gHasDisplayUpdatedForModeChange = 1;
	}
}

Now if I assume that alongside the rt_printf() statements you are also calling sendBuffer(), one starts to see where the issue could arise: you are checking a flag, doing something that takes time (sending to display) and then resetting the flag. If the audio thread in the meantime has set the flag again, that will go unnoticed, and you will have sent out the last-but-one state thinking you sent out the latest. I am surprised this happens, though, because as you are triggering writes with a button, there should be plenty of time for this thread to run before the next write is triggered.

This is a common pitfall of this simple flagging mechanism. One simple way to dramatically reduce the chances of that happening is to reset the flag immediately as you checked it, before any time-consuming operation:

void updateDisplays(void *) {
	while (!Bela_stopRequested()) {
		// early condition check, easier to read
		if (!gDisplaysInitialized || gHasDisplayUpdatedForModeChange)
		{
			usleep(gTaskSleepTime);
			continue;
		}
		// immediately reset the flag
		gHasDisplayUpdatedForModeChange = 1;
		// if we are here, we need to send a refresh
       		printf("MODE CHANGED\n");
		// do the time-intensive stuff
		if (gMode == kMode1) {
				rt_printf("drawing mode 1\n");
		} else if (gMode == kMode2) {
				rt_printf("drawing mode 32\n");
		}
	}
}

again, this is not thread-safe, in that the audio thread could preempt this thread after you checked the flag and before you reset it, but if gMode is updated before gHasDisplayUpdatedForModeChange (which requires special provisions in order to happen in C++ *), then you could be fine. Other options would be to have two flags: one global and one local to the auxiliary thread. The audio thread this time increases the global flag (instead of setting it to a fixed value) and the auxiliary thread "plays catch up" with it, e.g.:

volatile int gMode;
volatile int gFlag;

void updateDisplays(void *) {
	int myFlag = gFlag;
	while (!Bela_stopRequested()) {
		// early condition check, easier to read for me
		if (!gDisplaysInitialized || myFlag == gFlag)
		{
			usleep(gTaskSleepTime);
			continue;
		}
		// if we are here, we need to send a refresh
		myFlag = gFlag;
		if (gMode == kMode1) {
				rt_printf("drawing mode 1\n");
		} else if (gMode == kMode2) {
				rt_printf("drawing mode 32\n");
		}
	}
}

void render(BelaContext* context, void*)
{
	/// if button pressed ...
	gMode = newMode;
	gFlag++;
}

Again, because of *, it could still be that gMode is not update by the time you read gFlag ... The proper way of doing this would be to use C++11 atomics, or use a Pipe that is rt- and thread- safe. However, both would add some (however minimal) overhead to both threads. Using a Pipe, has the advantage that the display thread could be awaken exactly when needed (it wouldn't have to sleep and poll a global flag), but on the other hand if you send too many state changes too fast, it will have to read them all before being able to send out the latest, and each of these reads requires a system call to read from a virtual file, so there is a bit of overhead there, too.

In your case, though, if you only need to update the display if gMode has changed, the simplest (and this time foolproof) way of doing it would be to keep a copy of the state in the display thread and only send out if the current state is different from the last state that was sent to the display:

volatile int gMode;

void updateDisplays(void *) {
	int myMode = gMode;
	while (!Bela_stopRequested()) {
		// early condition check, easier to read for me
		if (!gDisplaysInitialized || myMode == gMode)
		{
			usleep(gTaskSleepTime);
			continue;
		}
		// if we are here, we need to send a refresh
		myMode = gMode;
		// from now on, use the local copy of the variable
		if (myMode == kMode1) {
				rt_printf("drawing mode 1\n");
		} else if (myMode == kMode2) {
				rt_printf("drawing mode 32\n");
		}
	}
}

void render(BelaContext* context, void*)
{
	/// if button pressed ...
	gMode = newMode;
}

In this case, all previous considerations still apply: it may be that by the time you are done writing to the screen in the display thread, the global variable has changed again, perhaps even more than once and so by the time you reach the last instruction of the while() loop, the display may already be out of sync with the global state. This time, however we can catch that case because our local copy myMode of the global variable will tell us at the next iteration of the loop that we need to refresh again. In this case, if the audio thread sets gMode in rapid succession to, e.g.: 1 0 1 0 1, the display thread may lose some of the transitions and so display e.g.: only 1 0 1 (dropping some frames), but it will always land on the last one that was set, which is probably what you want for this application. This means that after a button press (and/or several repeated ones), once you stop pressing buttons, the display will be up to date in a matter of a few milliseconds. Note that this trick works nicely because the global variable here is 4-byte long and fully represents the state. This method may not scale well when the global state becomes larger as the overhead of memory copy and comparing may become significant and/or the global state as seen by the display thread may become inconsistent while the audio thread is writing to it.

*: I am not an expert here, but keep in mind that in general simply writing

gMode = 1;
gHasDisplayUpdatedForModeChange = 0;

does not guarantee that the first write happens before the second and even less that another thread would see the results appear in that order. Declaring both global variables volatile would help in that sense, but you'd really need a memory barrier.

nickcamillo

Thanks, this is all very helpful.

I'm happy to post the full aux code, I'm actually separately very interested in your opinions on how to structure it because I've been totally hasty and haphazard about it, as it's been secondary in priority to implementing the actual functionality and I've been waiting to learn more before rewriting it properly. I was just trying to get to the bare minimum code to reproduce the issue I was having in order to eliminate other possibilities because there's a lot going on in the function, and the code that I posted above that only printed and didn't even write to the screen at all was indeed still having the issue that went away by always sleeping.

For context:

I've got 5 OLEDs. The first is usually displaying data or an instruction. The last 4 are either displaying a waveform with a playhead and a sequencer below, or information about keys to press, or a representation of a specific parameter as a bar across the 4 displays.

There are 6 modes, Looper, Sequencer, SoundLoader, SoundSaver, PatternLoader, and PatternSaver. Sequencer mode has sub-modes that are activated by holding down specific buttons. Surprisingly enough (to me at least), this all works pretty much perfectly, with the exception of drawing the parameter bars, which doesn't fire nearly as often as the trill sensors are reading. This is somewhat expected as even when changing modes and updating all 5 displays at once you can see them updating in order (pretty quickly, but visibly), but it's still rendering more slowly than I'd like, especially considering that which displays to update are optimized in the function for drawing the bar and only the ones with changed pixels are rendered. I'd say it renders the parameter bar once for every 10-15 trill reads / updates to values.

Would love any and all advice and direction you're willing to share and happy to share other bits of code that you want to see. I recognize some glaring issues like calling the getter functions at the top every time, which is already happening in the render function and could just be assigned to a global variable every tick and referenced here. I also really don't like how I'm handling blink updates. But yeah, I have been needing to redo this all properly for a while and IT IS TIME.

void updateDisplays(void *) {
    while (!Bela_stopRequested()) {
        if (gDisplaysInitialized == 1) {
        	if (gHasDisplayUpdatedForModeChange == 0 && gHasDisplayUpdatedForParameterChange == 0 && gHasDisplayUpdatedForSelectedStepChange == 0 && gHasDisplayUpdatedForBlinkChange == 0) {
				usleep(gTaskSleepTime);
				continue;
        	}
        	
            std::vector < Sample * > samples = gSequence.getSamples();
            std::vector < Step * > steps = gSequence.getSteps();
            Step * currentStep = gSequence.getCurrentStep();
            int currentStepDisplay = currentStep -> getDisplayIndex();

            int currentPlayheadDisplay = currentStep -> getPlayheadDisplayIndex();
            int stepBoxToDraw = 0;

			if (gMode == kSoundLoader) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundLoaderInfoPane();
					drawSoundLoaderInfoPane2(gSavedState);
					drawSoundLoaderInfoPane3(gSavedState);
					drawSoundLoaderInfoPane4(gSavedState);
					drawSoundLoaderInfoPane5(gSavedState);
					
					gHasDisplayUpdatedForModeChange = 1;
				}
			} else if (gMode == kSoundSaver) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundSaverInfoPane();
					drawSoundSaverInfoPane2(gSavedState);
					drawSoundSaverInfoPane3(gSavedState);
					drawSoundSaverInfoPane4(gSavedState);
					drawSoundSaverInfoPane5(gSavedState);
					
					gHasDisplayUpdatedForModeChange = 1;
				} else if (!gHasDisplayUpdatedForBlinkChange) {
					drawSoundSaverInfoPane();
					gHasDisplayUpdatedForBlinkChange = 1;
				}			
			} else if (gMode == kLooper) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawLooperInfoPane();
					drawLooperInfoPane2();
					drawLooperInfoPane3();
					drawLooperInfoPane4(gSavedState);
					drawLooperInfoPane5();

					gHasDisplayUpdatedForModeChange = 1;
				} else if (!gHasDisplayUpdatedForBlinkChange) {
					drawLooperInfoPane();
					drawLooperInfoPane2();
					gHasDisplayUpdatedForBlinkChange = 1;
				}
			} else if (gMode == kSequencer) {
				if (gSequencerMode == kSequencerMenu) {
   					if (!gHasDisplayUpdatedForModeChange) {
	   					drawSequencerMenuPane();
	  					drawSequencerMenuPane2();
	   					drawSequencerMenuPane3();
	   					drawSequencerMenuPane4();
	   					drawSequencerMenuPane5();
	   					
	   					gHasDisplayUpdatedForModeChange = 1;
   					}
   				} else if (gSequencerMode == kSequencerPattern) {
					if (!gHasDisplayUpdatedForSelectedStepChange) {
						drawSequencerPatternForSelectedStepChange(gSequence);
					} else if (!gHasDisplayUpdatedForModeChange) {
						drawSequencerInfoPanel(gSequence);
						drawAllSequencerDisplays(&stepBoxToDraw, currentStep->getIndex(), gSequence, steps, samples);
					} else if (gTransportState == kPlaying) {
						drawSequencerPatternForPlayback(currentPlayheadDisplay, currentStepDisplay, currentStep -> getIndex(), &stepBoxToDraw, steps, gSequence.getNumberOfSteps(), gSequence.getNumberOfSamples());
					}
		
		            gLastCurrentStepDisplay = currentStepDisplay;
   				} else if (gSequencerMode == kSequencerSampler) {
   					drawSequencerSamplerMenuPane();
   					drawAllSequencerDisplays(&stepBoxToDraw, currentStep->getIndex(), gSequence, steps, samples);

   				} else if (gSequencerMode == kSequencerParameter) {
   					if (gPatternParameter == kPatternParameterNone) {
   						drawSequencerParametersMenu();
   					} else {
   						drawSequencerParameter(gSong, gSequence);
   					}
   				}
			} else if (gMode == kPatternLoader) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundLoaderInfoPane();
					drawSoundLoaderInfoPane2(gSavedState);
					drawSoundLoaderInfoPane3(gSavedState);
					drawSoundLoaderInfoPane4(gSavedState);
					drawSoundLoaderInfoPane5(gSavedState);
					
					gHasDisplayUpdatedForModeChange = 1;
				}
			} else if (gMode == kPatternSaver) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundSaverInfoPane();
					drawSoundSaverInfoPane2(gSavedState);
					drawSoundSaverInfoPane3(gSavedState);
					drawSoundSaverInfoPane4(gSavedState);
					drawSoundSaverInfoPane5(gSavedState);
					
					gHasDisplayUpdatedForModeChange = 1;
				} else if (!gHasDisplayUpdatedForBlinkChange) {
					drawSoundSaverInfoPane();
					gHasDisplayUpdatedForBlinkChange = 1;
				}			
			} 
        }
    }
}

nickcamillo

Here's the code for drawing a parameter bar since I figure you will probably want to see it:

void drawParameterBar(float parameter, float parameterLast) {
    float parameterTimesFour = parameter * 4.0;
	float parameterLastTimesFour = parameterLast * 4.0;
    int numberOfPixels = 0;
    
    std::vector<int> numberOfPixelsUsedByDisplayLast{0, 0, 0, 0};
    std::vector<int> numberOfPixelsUsedByDisplay{0, 0, 0, 0};
    
    for (unsigned int d = 1; d < gParameterDisplays + 1; ++d) {
        if (parameterLastTimesFour >= d) {
        	numberOfPixels = gDisplayWidth;
    	} else if (parameterLastTimesFour <= d - 1) {
    		numberOfPixels = 0;
    	} else {
        	float splitRemainder = parameterLastTimesFour - (d - 1);
        	numberOfPixels = splitRemainder * gDisplayWidth;
        }
    	numberOfPixelsUsedByDisplayLast[d] = numberOfPixels;
        
        if (parameterTimesFour >= d) {
        	numberOfPixels = gDisplayWidth;
    	} else if (parameterTimesFour <= d - 1) {
    		numberOfPixels = 0;
    	} else {
        	float splitRemainder = parameterTimesFour - (d - 1);
        	numberOfPixels = splitRemainder * gDisplayWidth;
        }
    	numberOfPixelsUsedByDisplay[d] = numberOfPixels;
    }
    
    for (unsigned int d = 1; d < gParameterDisplays + 1; ++d) {    
        // rt_printf("gHasDisplayUpdatedForModeChange :%d\t parameterLast: %f\t paramter: %f\t displayWithEndOfBar: %d\t displayWithEndOfBar: %d\n", gHasDisplayUpdatedForModeChange, parameterLast, parameter, displayWithEndOfBar, displayWithEndOfBarLast);
   
        rt_printf("d: %d\t numPixels: %d\t numPixelsLast: %d\n", d, numberOfPixelsUsedByDisplay[d], numberOfPixelsUsedByDisplayLast[d]);

        
        if (gHasDisplayUpdatedForModeChange == 0) || numberOfPixelsUsedByDisplay[d] != numberOfPixelsUsedByDisplayLast[d]) {
	        switchTarget(d);
	        gU8g2->clearBuffer();

	        gU8g2->drawBox(0, 0, numberOfPixelsUsedByDisplay[d], gDisplayHeight);
	        gU8g2->sendBuffer();
    	}
    }	
}

giuliomoro

There's definitely room for improvement.

First, refactor for readability: instead of enumerating the function calls for each display, pack them all into a single call, e.g.: drawSoundLoaderInfoPanes() replacing all the various drawSoundLoaderInfoPanesX() calls.

void drawSoundLoaderInfoPanes()
{
	drawSoundLoaderInfoPane();
	drawSoundLoaderInfoPane2(gSavedState);
	drawSoundLoaderInfoPane3(gSavedState);
	drawSoundLoaderInfoPane4(gSavedState);
	drawSoundLoaderInfoPane5(gSavedState);
}

void updateDisplays(void *) {
	while (!Bela_stopRequested()) {
		if (gDisplaysInitialized == 1) {
			if (gHasDisplayUpdatedForModeChange == 0 && gHasDisplayUpdatedForParameterChange == 0 && gHasDisplayUpdatedForSelectedStepChange == 0 && gHasDisplayUpdatedForBlinkChange == 0) {
				usleep(gTaskSleepTime);
				continue;
			}

			std::vector < Sample * > samples = gSequence.getSamples();
			std::vector < Step * > steps = gSequence.getSteps();
			Step * currentStep = gSequence.getCurrentStep();
			int currentStepDisplay = currentStep -> getDisplayIndex();

			int currentPlayheadDisplay = currentStep -> getPlayheadDisplayIndex();
			int stepBoxToDraw = 0;

			if (gMode == kSoundLoader) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundLoaderInfoPanes(gSavedState);
					gHasDisplayUpdatedForModeChange = 1;
				}
			} else if (gMode == kSoundSaver) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundSaverInfoPanes(gSaveState);
					gHasDisplayUpdatedForModeChange = 1;
				} else if (!gHasDisplayUpdatedForBlinkChange) {
					drawSoundSaverInfoPane();
					gHasDisplayUpdatedForBlinkChange = 1;
				}
			} else if (gMode == kLooper) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawLooperInfoPanes(gSavedState);
					gHasDisplayUpdatedForModeChange = 1;
				} else if (!gHasDisplayUpdatedForBlinkChange) {
					drawLooperInfoPanes();
					gHasDisplayUpdatedForBlinkChange = 1;
				}
			} else if (gMode == kSequencer) {
				if (gSequencerMode == kSequencerMenu) {
   					if (!gHasDisplayUpdatedForModeChange) {
	   					drawSequencerMenuPanes();
	   					gHasDisplayUpdatedForModeChange = 1;
   					}
   				} else if (gSequencerMode == kSequencerPattern) {
					if (!gHasDisplayUpdatedForSelectedStepChange) {
						drawSequencerPatternForSelectedStepChange(gSequence);
					} else if (!gHasDisplayUpdatedForModeChange) {
						drawSequencerInfoPanel(gSequence);
						drawAllSequencerDisplays(&stepBoxToDraw, currentStep->getIndex(), gSequence, steps, samples);
					} else if (gTransportState == kPlaying) {
						drawSequencerPatternForPlayback(currentPlayheadDisplay, currentStepDisplay, currentStep -> getIndex(), &stepBoxToDraw, steps, gSequence.getNumberOfSteps(), gSequence.getNumberOfSamples());
					}

					gLastCurrentStepDisplay = currentStepDisplay;
   				} else if (gSequencerMode == kSequencerSampler) {
   					drawSequencerSamplerMenuPane();
   					drawAllSequencerDisplays(&stepBoxToDraw, currentStep->getIndex(), gSequence, steps, samples);

   				} else if (gSequencerMode == kSequencerParameter) {
   					if (gPatternParameter == kPatternParameterNone) {
   						drawSequencerParametersMenu();
   					} else {
   						drawSequencerParameter(gSong, gSequence);
   					}
   				}
			} else if (gMode == kPatternLoader) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundLoaderInfoPanes();
					gHasDisplayUpdatedForModeChange = 1;
				}
			} else if (gMode == kPatternSaver) {
				if (!gHasDisplayUpdatedForModeChange) {
					drawSoundSaverInfoPane();
					gHasDisplayUpdatedForModeChange = 1;
				} else if (!gHasDisplayUpdatedForBlinkChange) {
					drawSoundSaverInfoPane();
					gHasDisplayUpdatedForBlinkChange = 1;
				}
			}
		}
	}
}

gHasDisplayUpdatedForParameterChange is not used anywhere else in the loop, so probably not worth checking for it in the initial condition.

This looks like a bug:

if (!gHasDisplayUpdatedForSelectedStepChange) {
	drawSequencerPatternForSelectedStepChange(gSequence);
} else if (!gHasDisplayUpdatedForModeChange) {
	drawSequencerInfoPanel(gSequence);
	drawAllSequencerDisplays(&stepBoxToDraw, currentStep->getIndex(), gSequence, steps, samples);
}

these flags are checked but not reset, this means that while in this mode, after the display gets first updated, this thread will spin without sleeping until the mode is changed.

This one also looks fishy:

} else if (gMode == kPatternSaver) {
	if (!gHasDisplayUpdatedForModeChange) {
		drawSoundSaverInfoPane();
		gHasDisplayUpdatedForModeChange = 1;
	} else if (!gHasDisplayUpdatedForBlinkChange) {
		drawSoundSaverInfoPane();
		gHasDisplayUpdatedForBlinkChange = 1;
	}
}

you are checking for two different conditions but calling the same function.

In a few places you are using if() { } else if() { } where you could actually use if(){}; if() {}. For instance:

if (!gHasDisplayUpdatedForModeChange) {
	drawSoundSaverInfoPanes(gSaveState);
	gHasDisplayUpdatedForModeChange = 1;
} else if (!gHasDisplayUpdatedForBlinkChange) {
	drawSoundSaverInfoPane();
	gHasDisplayUpdatedForBlinkChange = 1;
}

but I see no reason why the second block should start with and else. Ultimately, you want your displays to be updated if the corresponding flag has been set. If multiple flags are found to be set at once, just service all of them:

if (!gHasDisplayUpdatedForModeChange) {
	drawSoundSaverInfoPanes(gSaveState);
	gHasDisplayUpdatedForModeChange = 1;
}
if (!gHasDisplayUpdatedForBlinkChange) {
	drawSoundSaverInfoPane();
	gHasDisplayUpdatedForBlinkChange = 1;
}

From an execution stand point, not much changes and your code will run pretty much the same. With the current code, when at least on flag is found to be set at the beginning of the loop, the thread doesn't sleep and if at the next iteration of the loop there is still one flag set (because only one was reset during the previous iteration), again it won't sleep. However, servicing all outstanding flag in a single iteration allows to simplify your code greatly by resetting them all unconditionally at the end of the loop. This also fixes the suspect bug above I pointed out above (flag read and not reset):

ovoid updateDisplays(void *) {
	while (!Bela_stopRequested()) {
		if (gDisplaysInitialized == 1) {
			if (gHasDisplayUpdatedForModeChange == 0 && gHasDisplayUpdatedForSelectedStepChange == 0 && gHasDisplayUpdatedForBlinkChange == 0) {
				usleep(gTaskSleepTime);
				continue;
			}

			std::vector < Sample * > samples = gSequence.getSamples();
			std::vector < Step * > steps = gSequence.getSteps();
			Step * currentStep = gSequence.getCurrentStep();
			int currentStepDisplay = currentStep -> getDisplayIndex();

			int currentPlayheadDisplay = currentStep -> getPlayheadDisplayIndex();
			int stepBoxToDraw = 0;

			if (gMode == kSoundLoader) {
				if (!gHasDisplayUpdatedForModeChange)
					drawSoundLoaderInfoPanes(gSavedState);
			} else if (gMode == kSoundSaver) {
				if (!gHasDisplayUpdatedForModeChange)
					drawSoundSaverInfoPanes(gSaveState);
				if (!gHasDisplayUpdatedForBlinkChange)
					drawSoundSaverInfoPane();
			} else if (gMode == kLooper) {
				if (!gHasDisplayUpdatedForModeChange)
					drawLooperInfoPanes(gSavedState);
				if (!gHasDisplayUpdatedForBlinkChange)
					drawLooperInfoPanes();
			} else if (gMode == kSequencer) {
				if (gSequencerMode == kSequencerMenu) {
   					if (!gHasDisplayUpdatedForModeChange) {
	   					drawSequencerMenuPanes();
	   					gHasDisplayUpdatedForModeChange = 1;
   					}
   				} else if (gSequencerMode == kSequencerPattern) {
					if (!gHasDisplayUpdatedForSelectedStepChange)
						drawSequencerPatternForSelectedStepChange(gSequence);
					if (!gHasDisplayUpdatedForModeChange) {
						drawSequencerInfoPanel(gSequence);
						drawAllSequencerDisplays(&stepBoxToDraw, currentStep->getIndex(), gSequence, steps, samples);
					}
					// TODO: double check that I didn't change the desired behaviour here by removing else if
					if (gTransportState == kPlaying)
						drawSequencerPatternForPlayback(currentPlayheadDisplay, currentStepDisplay, currentStep -> getIndex(), &stepBoxToDraw, steps, gSequence.getNumberOfSteps(), gSequence.getNumberOfSamples());
					gLastCurrentStepDisplay = currentStepDisplay;
   				} else if (gSequencerMode == kSequencerSampler) {
   					drawSequencerSamplerMenuPane();
   					drawAllSequencerDisplays(&stepBoxToDraw, currentStep->getIndex(), gSequence, steps, samples);
   				} else if (gSequencerMode == kSequencerParameter) {
   					if (gPatternParameter == kPatternParameterNone) {
   						drawSequencerParametersMenu();
   					} else {
   						drawSequencerParameter(gSong, gSequence);
   					}
   				}
			} else if (gMode == kPatternLoader) {
				if (!gHasDisplayUpdatedForModeChange)
					drawSoundLoaderInfoPanes();
			} else if (gMode == kPatternSaver) {
				if (!gHasDisplayUpdatedForModeChange)
					drawSoundSaverInfoPane();
				if (!gHasDisplayUpdatedForBlinkChange)
					drawSoundSaverInfoPane();
			}
			// reset all flags unconditionally
			gHasDisplayUpdatedForModeChange = 1;
			gHasDisplayUpdatedForSelectedStepChange = 1;
			gHasDisplayUpdatedForBlinkChange = 1;
		}
	}
}

Note that here you have the same pathological race conditions outlined in the previous post:
check flag
do something that takes a lot of flag
reset flag

In order to minimise chances of this being a problem, flags can be reset immediately after the check by caching their values in local variables. E.g.:

void updateDisplays(void *) {
	while (!Bela_stopRequested()) {
		if (gDisplaysInitialized == 1) {
			if (gHasDisplayUpdatedForModeChange == 0 && gHasDisplayUpdatedForSelectedStepChange == 0 && gHasDisplayUpdatedForBlinkChange == 0) {
				usleep(gTaskSleepTime);
				continue;
			}
			// cache globals to local
			bool hasDisplayUpdatedForModeChange = gHasDisplayUpdatedForModeChange;
			bool hasDisplayUpdatedForSelectedStepChange = gHasDisplayUpdatedForSelectedStepChange;
			bool hasDisplayUpdatedForBlinkChange = gHasDisplayUpdatedForBlinkChange;
			// reset all global flags unconditionally
			gHasDisplayUpdatedForModeChange = 1;
			gHasDisplayUpdatedForSelectedStepChange = 1;
			gHasDisplayUpdatedForBlinkChange = 1;
			// .... more code here
			// now only check against local and not against global
			if (gMode == kSoundLoader) {
				if (!hasDisplayUpdatedForModeChange)
					drawSoundLoaderInfoPanes(gSavedState);
			} else if (gMode == kSoundSaver) {
				if (!hasDisplayUpdatedForModeChange)
					drawSoundSaverInfoPanes(gSaveState);
				if (!hasDisplayUpdatedForBlinkChange)
					drawSoundSaverInfoPane();

			//.....rest of code here
	}
}

again, similar caveats to the previous post apply. An added advantage of using a local flag is that you can implement any thread-safety improvements right there without scattering them all over the condition checking.

As for more feedback on your code, I don't know enough about your application to know whether you could simplify the flag mechanism to a single flag and then inferring the rest from gMode. You could be using a switch statement instead of the various else if chain for gMode (although that's just a stylistic change that I don't feel strongly about).

Patterns like this :

drawSoundLoaderInfoPane();
drawSoundLoaderInfoPane2(gSavedState);
drawSoundLoaderInfoPane3(gSavedState);
drawSoundLoaderInfoPane4(gSavedState);
drawSoundLoaderInfoPane5(gSavedState);

are usually red flags for me: what is different between the various functions ? would you be able to refactor the implementation of the various drawSoundLoaderInfoPaneX() functions by having a single function that gets passed an index, e.g.:

for(unsigned int n = 0; n < displays.size(); ++n)
    drawSoundLoaderInfoPane(n, gSavedState);

would this simplify the code, removing (quasi-)duplicated lines or would it make it more complicated?

nickcamillo

@giuliomoro i've tried a bunch of different ways of optimizing the displays and i'm still struggling with this. I made a class based on some of your suggestions, and reorganized my display code in a DisplayManager class.

Here is my DisplayManager class, which keeps track of which displays should update, and manages updating them:

#include "Bela.h"
#include <string>
#include <vector>

#include "render.h"
#include "debouncedButton.h"
#include "display_shared.h"
#include "display_sequencer.h"
#include "display_looper.h"
#include "song.h"
#include "sequence.h"
#include "step.h"
#include "sample.h"
#include "displayManager.h"

DisplayManager::DisplayManager(int numDisplays) {
    displayUpdateFlags.resize(numDisplays, false);
}

void DisplayManager::markDisplayForUpdate(int displayIndex) {
	//rt_printf("-----------------\n");
	
    //rt_printf("Current display update flags: ");
    //for (int i = 0; i < displayUpdateFlags.size(); ++i) {
    //    rt_printf("%d ", static_cast<int>(displayUpdateFlags[i]));
    //}
	
    if (displayIndex >= 0 && displayIndex < displayUpdateFlags.size()) {
        displayUpdateFlags[displayIndex] = true;
    }
    
   rt_printf("flags after updating marked flags: ");
    for (int i = 0; i < displayUpdateFlags.size(); ++i) {
        rt_printf("%d ", static_cast<int>(displayUpdateFlags[i]));
    }
    rt_printf("-----------------\n");
}

void DisplayManager::markDisplaysForUpdate(std::initializer_list<int> displayIndices) {
    for (int displayIndex : displayIndices) {
        markDisplayForUpdate(displayIndex);
    }
}


bool DisplayManager::updateDisplays() {

	if (anyDisplayNeedsUpdate()) {	
	    rt_printf("Current display update flags at updateDisplays: ");
	    for (int i = 0; i < displayUpdateFlags.size(); ++i) {
	        rt_printf("%d ", static_cast<int>(displayUpdateFlags[i]));
	    }
	    rt_printf("\n");
	}
	bool updated = false;
	
    for (int i = 0; i < displayUpdateFlags.size(); ++i) {
        if (displayUpdateFlags[i]) {
		     // Print the current value of all flags
		    rt_printf("Current display update flags: ");
		    for (int i = 0; i < displayUpdateFlags.size(); ++i) {
		        rt_printf("%d ", static_cast<int>(displayUpdateFlags[i]));
		    }
		    rt_printf("\n");

        	rt_printf("updating display: %d\n", i);
            update(i);
            displayUpdateFlags[i] = false;
            
            updated = true;
        }
    }

	return updated;
}

bool DisplayManager::shouldUpdateDisplay(int displayIndex) const {
    if (displayIndex >= 0 && displayIndex < displayUpdateFlags.size()) {
        return displayUpdateFlags[displayIndex];
    }
    return false;
}

int getCurrentInfoDisplayMode() {
    int currentInfoDisplayMode = INFO_DISPLAY_MODE_SOUND;
    float currentParameterValue = 0;

rt_printf("gButtonLeft1.isHeld: %d\n", gButtonLeft1.isHeld());
    switch (gMode) {
    case kLooper:
        currentInfoDisplayMode = INFO_DISPLAY_MODE_SOUND;
        break;
    case kSequencer:
        if (gButtonLeft1.isHeld()) {
            currentInfoDisplayMode = INFO_DISPLAY_MODE_SEQUENCE_MENU;
        } else if (gButtonLeft2.isHeld()) {
            if (gIsParameterSelectionButtonPressed) {
                currentInfoDisplayMode = INFO_DISPLAY_MODE_PARAMETER;
                if (gPatternParameter == kPatternParameterVolume) {
                    currentParameterValue = gSequence.getVolume();
                } else if (gPatternParameter == kPatternParameterNumberOfChops) {
                    currentParameterValue = map(gSequence.getNumberOfSamples(), 2, 16, 0, 1);
                } else if (gPatternParameter == kPatternParameterNumberOfSteps) {
                    currentParameterValue = map(gSequence.getNumberOfSteps(), 2, 16, 0, 1);
                } else if (gPatternParameter == kPatternParameterPlaybackRate) {
                    currentParameterValue = gSequence.getPlaybackRate();
                } else if (gPatternParameter == kPatternParameterPlaybackType) {
                    currentParameterValue = map(gSequence.getPlaybackType(), 0, gPlaybackTypeList.size() - 1, 0, 1);
                } else if (gPatternParameter == kPatternParameterStartShift) {
                    currentParameterValue = gSequence.getStartShift();
                } else if (gPatternParameter == kPatternParameterTempo){ 
                    currentParameterValue = gSong.getTempo();
                }
            } else {
                currentInfoDisplayMode = INFO_DISPLAY_MODE_PARAMETER_MENU;
            }
        } else {    
            currentInfoDisplayMode = INFO_DISPLAY_MODE_SEQUENCE;
        }
        break;
    case kSoundLoader:
        currentInfoDisplayMode = INFO_DISPLAY_MODE_SOUND_LOADER;
        break;
    case kSoundSaver:
        currentInfoDisplayMode = INFO_DISPLAY_MODE_SOUND_SAVER;
        break;
    case kPatternLoader:
        currentInfoDisplayMode = INFO_DISPLAY_MODE_PATTERN_LOADER;
        break;
    case kPatternSaver:
        currentInfoDisplayMode = INFO_DISPLAY_MODE_PATTERN_SAVER;
        break;
    }
    
    return currentInfoDisplayMode;
}


void DisplayManager::updateInfoPane() {
	int infoDisplayMode = getCurrentInfoDisplayMode();
	ParameterData parameterData = getParameterData(&gSong, &gSequence, gPatternParameter);
	
	if (infoDisplayMode == INFO_DISPLAY_MODE_SOUND) {
		rt_printf("DRAW LOOPER INFO PANE\n");
		drawLooperInfoPane();
	} else if (infoDisplayMode == INFO_DISPLAY_MODE_SEQUENCE) {
		rt_printf("DRAW SEQUENCER INFO PANE\n");
		drawSequencerInfoPanel();
	} else if (infoDisplayMode == INFO_DISPLAY_MODE_SEQUENCE_MENU) {
		rt_printf("DRAW SEQUENCER MENU\n");
		drawSequencerMenu();
	} else if (infoDisplayMode == INFO_DISPLAY_MODE_PARAMETER_MENU) {
		rt_printf("DRAW PARAMETERS MENU PANE\n");
		drawSequencerParametersMenu1();
	} else if (infoDisplayMode == INFO_DISPLAY_MODE_PARAMETER) {
		rt_printf("DISPLAY PARAMETER INFO PANE\n");
		drawParameterPane(parameterData.label + parameterData.valueFormatted, parameterData.value, parameterData.numberOfTicks);
	}
}

void DisplayManager::update(int displayIndex) {
	
	if (displayIndex == 0) {
		updateInfoPane();
	} else if (gMode == kSoundLoader) {
		if (displayIndex == 1) {
			drawSoundLoaderInfoPane2();
		} else if (displayIndex == 2) {
			drawSoundLoaderInfoPane3();
		} else if (displayIndex == 3) {
			drawSoundLoaderInfoPane4();
		} else {
			drawSoundLoaderInfoPane5();
		}
	} else if (gMode == kSoundSaver) {
		if (displayIndex == 1) {
			drawSoundSaverInfoPane2();
		} else if (displayIndex == 1) {
			drawSoundSaverInfoPane3();
		} else if (displayIndex == 2) {
			drawSoundSaverInfoPane4();
		} else {
			drawSoundSaverInfoPane5();
		}
	} else if (gMode == kLooper) {
		if (displayIndex == 1) {
			drawLooperInfoPane2();
		} else if (displayIndex == 2) {
			drawLooperInfoPane3();
		} else if (displayIndex == 3) {
			drawLooperInfoPane4();
		} else {
			drawLooperInfoPane5();
		}
	} else if (gMode == kSequencer) {
		Step * currentStep = gSequence.getCurrentStep();

		drawSequencerDisplay(displayIndex, currentStep->getIndex(), currentStep->getPlayheadDisplayIndex() == displayIndex);
	} else if (gMode == kPatternLoader) {
		if (displayIndex == 1) {
			drawPatternLoaderInfoPane2();
		} else if (displayIndex == 2) {
		} else if (displayIndex == 3) {
			drawPatternLoaderInfoPane4();
		} else {
			drawPatternLoaderInfoPane5();
		}
	} else if (gMode == kPatternSaver) {
		if (displayIndex == 1) {
			drawPatternSaverInfoPane2();
		} else if (displayIndex == 2) {
			drawPatternSaverInfoPane3();
		} else if (displayIndex == 3) {
			drawPatternSaverInfoPane4();
		} else {
			drawPatternSaverInfoPane5();
		}
	}
}

std::vector<bool> DisplayManager::getDisplayUpdateFlags() {
	return displayUpdateFlags;
}

bool DisplayManager::anyDisplayNeedsUpdate() const {
    for (bool flag : displayUpdateFlags) {
        if (flag) {
            return true;
        }
    }
    return false;
}

The updateDisplays method is being called in the aux function:


void updateDisplays(void *) {
    while (!Bela_stopRequested()) {
    	if (!gDisplayManager.updateDisplays()) {
			usleep(gTaskSleepTime);
    	}
    }
}

In my render loop I am marking specific displays to update when the current step changes or the playhead updates.

            if (gTransportState == kPlaying && (gSong.advanceMetronomeCounter() >= gSong.getMetronomeInterval())) {
                    gSong.setMetronomeCounter(0);

                    gSequence.advance();
                    gSequence.trigger();
                    int currentStepIndex = gSequence.getCurrentStepIndex();
                    int displayIndex = (currentStepIndex / gStepsPerDisplay) % gTotalWaveformDisplays;
                    displayIndex += 1;

				    int previousSelectedIndex = currentStepIndex - 1;
				    if (previousSelectedIndex < 0) {
				        previousSelectedIndex += (gTotalWaveformDisplays * gStepsPerDisplay);
				    }
				    int previousDisplayIndex = (previousSelectedIndex / gStepsPerDisplay) % gTotalWaveformDisplays;
					previousDisplayIndex += 1;
					//rt_printf("displays marked for update: %d\n")
					rt_printf("marking displays for current step update: %d, %d\n", displayIndex, previousDisplayIndex);
                    gDisplayManager.markDisplaysForUpdate({displayIndex, previousDisplayIndex});
            }

and

			        // Calculate the current display index containing the playhead
				    int currentDisplayIndex = (gPlayheadX / gDisplayWidth) % gTotalWaveformDisplays;
				    currentDisplayIndex += 1;
				
				    // Calculate the previous playhead position
				    int previousPlayheadX = gPlayheadX - 1;
				    if (previousPlayheadX < 0) {
				        previousPlayheadX += (gTotalWaveformDisplays * gDisplayWidth);
				    }
				
				    // Calculate the previous display index
				    int previousDisplayIndex = (previousPlayheadX / gDisplayWidth) % gTotalWaveformDisplays;
				    previousDisplayIndex += 1;
				
					rt_printf("marking displays for playhead update: %d ,%d\n", currentDisplayIndex, previousDisplayIndex);
				    gDisplayManager.markDisplaysForUpdate({currentDisplayIndex, previousDisplayIndex});

I've set up print statements to validate that the displayUpdate flags are being set correctly at the correct times. For example, when step 4 in the sequencer transitions to step 5, which is on the next display, it is correctly marking both display 1 and display 2 as ready for update. i.e. 01100

That said, when the aux function gets around to actually updating, when I print out the flags (which happens after verifying that the flags were set to the values above), I am getting 00100, and therefore the previous display that should no longer display the current step does not get updated.

When I comment out the code that is updating the flags for the playhead changing, the currentStep code works as expected. This made me think that maybe I was updating the flags wrong, but I'm not resetting any flags to false until they are actually rendered.

What do you think is going on?

nickcamillo

It seemed unintuitive to me, because of order of the print logs, but it was a race condition that was causing this. At first I tried a mutex on each of the functions, but that just caused stuttering in the audio thread. So instead, I added mutexes to the functions that are being called from the audio thread (to block the display thread), and used a double buffer system to ensure that each thread accesses different sets of flags:

int NUM_DISPLAYS = 5;

DisplayManager::DisplayManager() {
    displayUpdateFlagsFront.resize(NUM_DISPLAYS, false);
    displayUpdateFlagsBack.resize(NUM_DISPLAYS, false);
}

void DisplayManager::markDisplayForUpdate(int displayId) {
    std::lock_guard<std::mutex> lock(displayUpdateFlagsMutex);
    displayUpdateFlagsBack[displayId] = true;
    shouldSwapBuffers = true;
}

void DisplayManager::markDisplaysForUpdate(const std::vector<int>& displayIds) {
    std::lock_guard<std::mutex> lock(displayUpdateFlagsMutex);
    for (const int displayId : displayIds) {
        displayUpdateFlagsBack[displayId] = true;
    }
    shouldSwapBuffers = true;
}

void DisplayManager::swapBuffers() {
    std::lock_guard<std::mutex> lock(displayUpdateFlagsMutex);
    if (shouldSwapBuffers) {
        displayUpdateFlagsFront.swap(displayUpdateFlagsBack);
        std::fill(displayUpdateFlagsBack.begin(), displayUpdateFlagsBack.end(), false);
        shouldSwapBuffers = false;
    }
}

bool DisplayManager::updateDisplays() {
    swapBuffers();
    
    bool updated = false;

    for (int displayId = 0; displayId < displayUpdateFlagsFront.size(); ++displayId) {
        if (displayUpdateFlagsFront[displayId]) {
            rt_printf("Current display update flags: ");
            for (int i = 0; i < displayUpdateFlagsFront.size(); ++i) {
                rt_printf("%d ", static_cast<int>(displayUpdateFlagsFront[i]));
            }
            rt_printf("\n");
            
            update(displayId);
            displayUpdateFlagsFront[displayId] = false;
            updated = true;
        }
    }
    
    return updated;
}

nickcamillo

@giuliomoro the above changes are working really well for me, it's the fastest I've been able to get these displays to update so far and a marked improvement. that said, when i have to update many at once, you can still see them updating in order.

If I were to create a separate aux thread for each display, each only checking whether to update itself, is it plausible that I could get them to update simultaneously?

giuliomoro

nickcamillo the above changes are working really well for me, it's the fastest I've been able to get these displays to update so far and a marked improvement. that said, when i have to update many at once, you can still see them updating in order.

nice!

nickcamillo If I were to create a separate aux thread for each display, each only checking whether to update itself, is it plausible that I could get them to update simultaneously?

This is impossible because they are all using the same I2C bus. As there are 2 I2C busses available on the headers, you could consider splitting your displays over the 2 different busses, making sure that each display from the display pairs whose synchronised update you care about the most are on two separate busses. At that point having two threads, one per I2C bus, should improve the synchronisation significantly.

« Previous Page Next Page »