Happy little Incident #8

[Mcu32]

int AudioConnection::connect(void):

1:
Incomplete loop (while (p->next_dest)):

The while condition terminates as soon as p->next_dest == NULL. As a result, the last element in the list is never checked for a duplicate.

2:
Incorrect pointer comparison (&p->src == &this->src):

while (p->next_dest)  // scan source Stream's connection list for duplicates{    if (&p->src == &this->src && &p->dst == &this->dst        && p->src_index == this->src_index && p->dest_index == this->dest_index)    {

&p->src takes the address of the pointer member src within object p (AudioStream**). Since p and this are two distinct AudioConnection objects in memory, &p->src == &this->src is always false. The pointer values themselves must be compared (p->src == src or p->src == this->src).



receiveWritable()

needs - following the ususal pattern - a __disable_irq();



No examples here; I don't think it's necessary, and it would be unnecessary work.

 

[h4yn0nnym0u5e]

1. I believe AI has spotted an error here, but even if corrected the check is redundant, as the potential duplicate connection being checked for will already have been rejected by a previous simpler check for the destination being used twice. Messy and embarrassing (I wrote this…), but not going to result in obscure sketch bugs, I believe. If you can do some “unnecessary work” and come up with a sketch which fails because of this, I’m happy to change my mind; I’m pretty sure your time would be wasted, though, because I don’t think it’s possible.

2. No. receiveWritable(), receive() and transmit() are only ever used inside update(), which is already executed in a low-priority ISR. Any other use would constitute misuse. The higher-priority audio I/O ISRs can’t call any of these, since they’re not static methods.

 

[Mcu32]

No, if you say it's a waste of time, okay, then I won't waste my time on it.
But I took a quick look and checked “receiveWritable.”

I found it here:
Suche "receiveWritable" (4 Treffer in 3 Dateien von 161 gesucht) [Normal]
.platformio\packages\framework-arduinoteensy\cores\teensy4\AudioStream.cpp (1 Treffer)
Zeile 187: audio_block_t * AudioStream::receiveWritable(unsigned int index)
.platformio\packages\framework-arduinoteensy\cores\teensy4\AudioStream.h (1 Treffer)
Zeile 166: audio_block_t * receiveWritable(unsigned int index = 0);
.platformio\packages\framework-arduinoteensy\cores\teensy4\usb_audio.cpp (2 Treffer)
Zeile 333: left = receiveWritable(0); // input 0 = left channel
Zeile 334: right = receiveWritable(1); // input 1 = right channel

Maybe you could check to see if it's being used correctly in usb_audio?

Question: Do you know why all interrups are disabled, - or if would be ok to to diable the audio interrupt only?
I'm out of the loop on this topic; it's been too long.

 

[Mcu32]

Wrong alert: it's used in update() in usb_audio, too. My fault.

Still, the comment is interesting:

    // TODO: we shouldn't be writing to these......
    //left = receiveReadOnly(0); // input 0 = left channel
    //right = receiveReadOnly(1); // input 1 = right channel
    left = receiveWritable(0); // input 0 = left channel
    right = receiveWritable(1); // input 1 = right channel

 

[h4yn0nnym0u5e]

As I believe has been frequently discussed, often involving your good self, the blanket disabling and re-enabling of interrupts is a highly questionable practice! I usually try quite hard to order my code such that it will function OK even if interrupted - my first job out of university was working on the Sinclair QL, and I rapidly became conscious of the need to write clean code in a multi-tasking environment. Few are so conscientious, and the blunt tools provided don't make it easy - a properly-written nesting disable/enable_irq() pair would be a great start.

Hmm, yes, that is an interesting comment. I can think of no reason why receiveWritable() is used here. In the revised multi-speed multi-channel USB code, @alex6679 has used receiveReadOnly() and it works extremely well. Maybe it's due to excess caution, or a legacy bug? A quick check of the blame on github shows the Teensy 4.x code is 6 years old, and that section (at least...) is a verbatim copy of the 9-year-old Teensy 3.x code.

 

[Mcu32]

I totally agree with you. Among other things, this destroys one of the biggest advantages a microcontroller has – deterministic behavior and quick response to more important interrupts (pin, etc...) - even if done right.
That's why I'm asking for your opinion on whether it might be possible to at least limit these blocks to the audio interrupt (instead disable for _all interrupts)
Unfortunately, my attempts (many..) to implement other methods have always been blocked.

There are also a few race conditions in the USB code, as well as problems in the serial code—but I won’t even get started/mention on those... (not wanted) Especially when you’re asked to provide an example that shows the problem.
My enthusiasm doesn’t go that far, especially since you rarely get any feedback at all on the work you’ve done.
I'm really curious to see if the other issues I mentioned—some of which I suggested solutions for, but all of which are easy to fix—will ever be resolved.

receiveWritable(): My only guess is, he tried receiveReadOnly, but it did not work - for whatever reason.