Bugs-AI

[Mcu32]

I would have an AI check the entire Teensy Core code. A quick test with AudioStream.cpp revealed a few bugs and areas for improvement. I only checked that one file, but I think it's worth doing this for the entire Core.

 

[Mcu32]

found some more in eventresponder (severeal races), in DMAChannel.cpp (sorting + several races)

 

[AaronD]

I've been looking through DMAChannel myself, getting ready to use it without the easy online documentation that some of the other libraries have.

Yeah, the priority sorter is pretty clearly not meant to be optimized. It works, it's easy to glance at and see that it works, and it probably only runs once anyway and so the performance hardly matters. I'd say, "no attention needed," on that.

I'd be interested in what else you found though. Coming at it with only a not-so-clear datasheet to check it with and no direct experience, I hope there aren't any weird gotchas to account for.

 

[Mcu32]

void DMAPriorityOrder(DMAChannel &ch1, DMAChannel &ch2, DMAChannel &ch3, DMAChannel &ch4)
{
    if (priority(ch3) < priority(ch4)) swap(ch3, ch4);
    if (priority(ch2) < priority(ch3)) swap(ch2, ch3);
    if (priority(ch1) < priority(ch2)) swap(ch1, ch2);
    if (priority(ch3) < priority(ch4)) swap(ch2, ch3);
    if (priority(ch2) < priority(ch3)) swap(ch1, ch2);
    if (priority(ch3) < priority(ch4)) swap(ch2, ch3);
}

This is probably supposed to be a manual bubble sort. However, in the last three lines, different values are being compared rather than swapped—probably a copy-and-paste error.

I'd say this is better and needs a line less:

void DMAPriorityOrder(DMAChannel &ch1, DMAChannel &ch2,
                      DMAChannel &ch3, DMAChannel &ch4)
{
    if (priority(ch1) < priority(ch2)) swap(ch1, ch2);
    if (priority(ch3) < priority(ch4)) swap(ch3, ch4);
    if (priority(ch1) < priority(ch3)) swap(ch1, ch3);
    if (priority(ch2) < priority(ch4)) swap(ch2, ch4);
    if (priority(ch2) < priority(ch3)) swap(ch2, ch3);
}

However, the races are not less important.

 

[Mcu32]

priority sorter is pretty clearly not meant to be optimized

Yes? Why do you think that?

performance hardly matters

yes.
I'd probably just use two nested loops. With -O3 (or, maybe, with -O2, too) the compiler will remove them anyway

 

[jmarsh]

The silly part here is that the swap function could just swap the priorities instead of swapping the channel assignments and TCD, which would avoid breaking existing cross-channel linkages or attached interrupts.

 

[AaronD]

in the last three lines, different values are being compared rather than swapped—probably a copy-and-paste error

Huh. Yeah you're right. Oops!

However, the races are not less important.

What are they? In a free-help community like this, saying there are problems but not saying what, hurts your credibility all around. So I looked up the original to see that you hadn't fabricated it. You hadn't, but there's still some doubt on the things you haven't explained yet.

priority sorter is pretty clearly not meant to be optimized

Yes? Why do you think that?

I explained a lot of it immediately after the part that you quoted:

it's easy to glance at and see that it works, and it probably only runs once anyway and so the performance hardly matters

In other words:

• It's (too?) easy to verify.
  • The obvious pattern creates a false sense of correctness, so the actual check doesn't happen while being mentally marked as if it did and passed.
• Like all the other hardware functions, the DMA also "just runs" after it's set up and needs no further attention except for the occasional explicit trigger for things that couldn't be done in hardware. And this is part of the setup. Thus, only running once.

So technically, it *is* optimized: to be pleasing for a human to look at and not necessarily for performance by any metric.

The silly part here is that the swap function could just swap the priorities instead of swapping the channel assignments and TCD, which would avoid breaking existing cross-channel linkages or attached interrupts.

Yeah I was wondering that too. Why swap the entire configs, when the single-number priorities can be assigned arbitrarily? It's an error for two of them to have the same priority, which is checked by the hardware and must be dealt with, so maybe swapping configs avoids that? Better to transfer something twice and something else not at all (potential race condition, if a trigger happens during the swap), than to set off the hardware error-detector?

Maybe I'm missing something - don't completely understand all of it yet - but so far, I really don't see a problem with clearing the error flag before returning. Beyond trading one race for another, I guess: if a priority error still exists when a trigger comes in...

Not that we can do anything about it, but I wonder why it's an error in the first place. Why not use the channel numbers as a tie-breaker and keep running? But that would be a hardware fix, and nothing that we can do in software. So, just make sure they're all unique, clear the error flag after changing them, and understand that some transfers may not have happened because of that error. I'd call that better than the possibility of corrupted transfers (who knows what *those* do!) because they were triggered mid-swap of their configs...

 

[Mcu32]

saying there are problems but not saying what, hurts your credibility all around

Well, I'd say it's even worse not to just do what the title of this thread says and expect others to do the work for you.
Just ask Gemini or any ai of your choice, give them the source code, and ask them to look for bugs. If that’s too much for you, I can’t help you anyway.

It’s just too much to go over here, and it affects not only the files mentioned but many more.

Let my credibility be my concern.

 

[AaronD]

Well, I'd say it's even worse not to just do what the title of this thread says and expect others to do the work for you.
Just ask Gemini or any ai of your choice, give them the source code, and ask them to look for bugs. If that’s too much for you, I can’t help you anyway.

It’s just too much to go over here, and it affects not only the files mentioned but many more.

Let my credibility be my concern.

It's more a sign that you don't understand the community. Regardless of how you found the problems or how many there are, you're basically saying that you know and won't say. Are you going to put that knowledge behind a paywall, perhaps, and this thread is your way of marketing it?

Anyway, yes, I can do the same as you did. That's fine. But the expectation here is to help the community by sharing what you found so the rest of us can build on your work instead of repeating it.

 

[jmarsh]

Yeah I was wondering that too. Why swap the entire configs, when the single-number priorities can be assigned arbitrarily?

Because (speaking about the T4.x implementation here) it was inherited from earlier platforms, some of which didn't have configurable priorities and strictly prioritised by channel number instead.
This inheritance/"carry-over" is responsible for several other bugs/limitations, for example only 16 out of the IMXRT1062's 32 channels are available to be used. I really don't understand why, since the code is maintained completely separate from Teensy3...

 

[Mcu32]

@Aaron: Well, I'm curious to see what you find and what pull requests you'll create. And also, whether any of them will be accepted. Let's hope so.
I guess I'm still one of the people with the most PRs, though my last one was many years ago.

 

[Mcu32]

I really don't understand why

I was one of the people who helped port all the code from T3 back then. It was a huge amount of work. I guess these things were just low-hanging fruit and the easiest way to keep everything compatible. One of the goals was that T3 Code runs on T4. For most of it, it worked.

But it wouldn't be too difficult to improve it now. If it can't be improved in a portable way, you could always create a “V2” of the API.

 

[Mcu32]

@Aaron for you:
(My AI speaks German)

Obwohl versucht wurde, mit __disable_irq() kritische Abschnitte zu schützen, entstehen durch die Platzierung der Interrupt-Freigabe und fehlendes Primask-Handling Sicherheitslücken beim gleichzeitigen Zugriff (z. B. Hauptschleife vs. Interrupts oder Multithreading).

1. Read-Modify-Write Race Condition auf Hardware-Registern​

In DMAChannel::begin() werden die Interrupts direkt nach dem Reservieren der Kanalbitmaske wieder aktiviert (__enable_irq()). Danach folgen Read-Modify-Write-Zugriffe (|=) auf gemeinsame Hardware-Register:

C++

// Interrupts werden HIER bereits wieder aktiviert:
__enable_irq();
break;

// ...

// RACE CONDITION: Nicht-atomare Lese-Modifiziere-Schreib-Zugriffe!
CCM_CCGR5 |= CCM_CCGR5_DMA(CCM_CCGR_ON);
dchpri[...] |= DMA_DCHPRI_ECP | DMA_DCHPRI_DPA;

Das Problem:Das Register CCM_CCGR5 steuert die Taktversorgung für mehrere Peripheriegeräte im Chip. Wenn ein Interrupt genau zwischen dem Lese- und Schreibschritt von CCM_CCGR5 |= ... unterbricht und dort ebenfalls ein Taktregister geändert wird, geht eine der beiden Änderungen verloren.

2. Inkonsistenter Objektdaten-Zustand in release()​

In release() erfolgt die Freigabe der globalen Maske vor dem Zurücksetzen der Objekteigenschaften:

C++

void DMAChannel::release(void)
{
if (channel >= DMA_MAX_CHANNELS) return;
DMA_CERQ = channel;
__disable_irq();
dma_channel_allocated_mask &= ~(1 << channel); // Kanal global freigegeben
__enable_irq(); // Interrupts wieder an

// RACE CONDITION: Das Objekt hält kurzzeitig noch den alten Kanal,
// obwohl die Maske ihn bereits als "frei" flaggt!
channel = DMA_MAX_CHANNELS;
TCD = (TCD_t *)0;
}

Das Problem:Wenn unmittelbar nach __enable_irq() ein ISR ausgeführt wird, der begin() aufruft, kann dieser denselben Kanal sofort neu zugewiesen bekommen. Greift der aufrufende Kontext danach noch auf das alte DMAChannel-Objekt zu, bevor channel = DMA_MAX_CHANNELS ausgeführt wurde, greifen zwei Objekte auf denselben DMA-Kanal zu.

3. Zerstörung übergeordneter kritischer Sektionen (Nesting Error)​

Der Code nutzt __disable_irq() und __enable_irq() bedingungslos:

C++

__disable_irq();
// ...
__enable_irq(); // Schaltet Interrupts bedingungslos wieder EIN

Das Problem:Wird begin() oder release() aus einem Kontext aufgerufen, in dem Interrupts bereits bewusst deaktiviert waren (z. B. innerhalb eines Interrupt-Handlers / ISRs oder einer eigenen kritischen Sektion), schaltet __enable_irq() die Interrupts global wieder ein. Das bricht den Schutz des aufrufenden Codes auf.

Richtiger Ansatz: Den aktuellen Status des PRIMASK-Registers sichern und wiederherstellen (Save & Restore).

4. Fehlende volatile-Deklaration bei dma_channel_allocated_mask​

C++

uint16_t dma_channel_allocated_mask = 0;

Da auf diese Variable aus normalen Funktionen und potenziell aus ISRs zugegriffen wird, muss sie als volatile deklariert sein. Andernfalls kann der Compiler Zugriffe darauf in Register cachen, wodurch Änderungen aus anderen Kontexten nicht gesehen werden.

 

[jmarsh]

2 is definitely a non-issue; the DMAChannel object calling release() isn't using its channel any more so it doesn't matter if it gets re-assigned in the very brief period between interrupts being re-enabled and the

channel = DMA_MAX_CHANNELS;

statement.

4 is also a non-issue: all access to dma_channel_allocated_mask is done with interrupts disabled so it's impossible for any other context to modify it during that time. The interrupt disable/enable calls contain volatile statements that prevent the compiler from reordering any accesses before or after them.

If you disagree, ask the AI to write sample code to conclusively demonstrate the issues.

 

[Mcu32]

This was the answer of the machine.
Yep, you always have to take what aIs say with a grain of salt. Still, they're helpful if you know how to work with them.

In addition, as a general rule, you should always check to see if there isn't a better way than constantly enabling and disabling interrupts system-wide.

 

[AaronD]

In addition to @jmarsh's comments, I'd offer that:

3 is also a non-issue. If we're still using the Teensyduino framework, then system-wide interrupts are assumed to always be enabled and so it's okay to unconditionally re-enable them after a critical section. In general yes, you would want to save the actual status of that bit and restore it, but here it's not necessary.

1 is okay too, if the framework does not enable global interrupts until after the setup is complete, which is the only time I'd expect the "problem" code to run. Thus, single-threaded at that point, no interrupts, and no race.

If you're dynamically beginning and ending stuff while running, then you're asking for so many other issues that this is pretty far down on your list of concerns. Just don't do that. Find a way to statically allocate everything that possibly can, even if it leads to less efficient memory usage. This is embedded, not desktop.

 

[PaulStoffregen]

We've long had an informal expectation that bug reports need to show a complete program & details to reproduce the problem. My feeling is this applies equally whether 100% organic human or AI assisted.

When this forum ran on vBulletin we even had a "Forum Rule" at the top of every page, requiring complete code. That didn't come over to XenForo partly because XF works differently, but also partly because the old Forum Rule sometimes results in hostile responses when people didn't follow it (and I'll admit, some of those may have been me...) Online community building is a tricky art.

Of course general conversation about bugs is allowed without specific requirements. So is conversation about general electronics. Casual conversation is fine.

But for a bug report to be taken seriously, generally speaking, a reproducible test case is expected.

However, there are exceptions. I do care about subtle bugs and possible race conditions, which I understand are extremely difficult to reproduce. Odds of me immediately responding are nearly zero, but when I see a message reporting a problem, often I will save a bookmark to look at it more carefully when I have time.

I do try to read every forum message. Sometimes on highly active threads (recent wifi stuff) I don't always keep up. But I do prioritize reading threads like this one, which are about possible bugs in the existing libraries. I really do appreciate everyone effort and especially analysis. If there really is a bug in need of my attention, I really want to know about it.

 

[Mcu32]

then system-wide interrupts are assumed to always be enabled

If you think that's the right way to do it and that it's clean code… well, fine, okay. I disagree. Keep an eye out for other bugs and fix them.

@PaulStoffregen; I started this thread to show a simple way to improve Teensyduino and make it more stable—as a helpful suggestion. If that’s not what you want, or if fixing race conditions or bugs is too much work, Paul, then just ignore it. At least, fix the sorting - or do you really need an example that the sorting is wrong? really? Test Case???

No. Read the code.

And at least consider that there are some crashes where no one can figure out the cause. Especially not experiences users. These are exactly those kinds of situations.

Ok, next sourcefile, then. Have fun writing test cases

 

[jmarsh]

Personally I think it's more productive to use time fixing existing bugs than hunting for imaginary ones. Hence... test cases, please.

 

[AaronD]

If you think that's the right way to do it and that it's clean code… well, fine, okay. I disagree. Keep an eye out for other bugs and fix them.

In general, I'm with you. Restore the original state instead of unexpectedly enabling.

But having seen a little bit of how the Arduino framework specifically is used, along with some of the modules for it, I get the impression that interrupts should always stay enabled to avoid weird "not working" bugs, that a beginner who followed all the top-level docs and gets lost beyond them can't figure out.

If you're capable of owning the entire codebase yourself, and do own the entire codebase yourself, then yes, it's much better practice to save and restore. But here specifically, I think the way it's presently done is actually better for beginners who only "add their code here" and just want their plug-in black-boxes to "just work" with no troubleshooting at all that they're not capable of anyway. So, casually enforcing the environment that those modules rely on, ends up being welcome.

Just be aware that if you use this framework, it does enforce that environment. If you don't like it, replace it with something that you will own.

 

[Mcu32]

try to sort 0, 3, 1, 2. it gives 3 ,0 ,1 ,2


for the other Problems: Again: Just don't fix them, if you're ok with that.

Just a general thought:
Unfortunately, the “if it ain't broke, don't fix it” approach doesn't work in the real world. To apply it, you have to know if something is broken—and that's hard to prove and often depends on the user's code. Maybe it works 999 out of 1,000 times—but not in that one case.

But whatever. I don't have to convince anyone. It was just a suggestion—do whatever you want with it and live with shaky code.
Bye from this thread

 

[PaulStoffregen]

Just to confirm, I've added DMAPriorityOrder on my list of issues to address in version 1.63.

 

[Mcu32]

Solution in post #4.

 

[AaronD]

Anything wrong with this? (addressing taken from the end of DMAChannel::begin):

static void DMA_Set_Priority(DMAChannel &c, uint8_t priority)
{
    volatile uint8_t *dchpri = (&DMA_DCHPRI3) + ((c.channel & 0x1C) | (3 - (c.channel & 0x03)));
    *dchpri = (*dchpri & 0xF0) | (priority & 0x0F);
}

Scrap the sorting, and do that instead? (after an independent bug-check)

Or perhaps take a uint8_t[DMA_MAX_CHANNELS], and verify that they're all unique and in-range before applying all? (still don't sort)

 

[Mcu32]

First, you would need to establish whether any user code relies on the channel assignments being changed.

You can't do that.

Therefore, your proposal would be a breaking change and would not be acceptable.