This code is so broken it some how fixes itself.

[paulca]

So this code works.  It shouldn't, but it does.  (Friend wrote it, acchmm).  I just find it amusing how broken it is that it seems to fix itself.

The SAI's are in 16bit extended.

The for loop is nuts.  There are not 8 16bit ints there.  There are 8 8bit ints.  Yet it works.  In fact a whole bunch of different viarities work also.

      for (int i = 0; i < 8; i+=4)

Is my favourite.

      for (int i = 0; i < 2; i++)  // Nope , not even with a int32_t buffer.
      for (int i = 0; i < 4; i++)  // Nope
      for (int i = 0; i < 8; i++)  // YES
      for (int i = 0; i < 8; i+=2)  // YES
      for (int i = 0; i < 8; i+=4)  // YES

I think it's just lucky word alignment.

Code: [Select]

int16_t buffer[8];

float gain = 0.75f;
while (1) {
HAL_SAI_Receive(&hsai_BlockA1, (uint8_t*) buffer, 8, 10);
for (int i = 0; i < 8; i++) {

double sample = buffer[i];
sample = sample * gain; // pre-gain
sample = PeakFilter_Update(&bassBoost, sample);
sample = PeakFilter_Update(&midCut, sample);
sample = PeakFilter_Update(&highBoost, sample);

buffer[i] = (int16_t) sample;
}
HAL_SAI_Transmit(&hsai_BlockA2, (uint8_t*) buffer, 8, 10);
}

EDIT: I'm laughing at myself but this is a classic example.  Classic.  Your code works first time.  "That was easy".  Never pay any attention.  "Some time later...", "That doesn't look right... I'll take a look...."

"Wait.  How does this work?  How did this ever work?  What the hell?"

No... I'm not looking for help to fix it.  I can see a symetry of problems that if fixes will make it make sense, but I just found it funny.  Symmetrical insanity just happens to work.

EDIT2: Oh and it has many other issues to boot.

[Sherlock Holmes]

So this code works.  It shouldn't, but it does.  (Friend wrote it, acchmm).  I just find it amusing how broken it is that it seems to fix itself.

The SAI's are in 16bit extended.

The for loop is nuts.  There are not 8 16bit ints there.  There are 8 8bit ints.  Yet it works.  In fact a whole bunch of different viarities work also.

      for (int i = 0; i < 8; i+=4)

Is my favourite.

      for (int i = 0; i < 2; i++)  // Nope , not even with a int32_t buffer.
      for (int i = 0; i < 4; i++)  // Nope
      for (int i = 0; i < 8; i++)  // YES
      for (int i = 0; i < 8; i+=2)  // YES
      for (int i = 0; i < 8; i+=4)  // YES

I think it's just lucky word alignment.

Code: [Select]

int16_t buffer[8];

float gain = 0.75f;
while (1) {
HAL_SAI_Receive(&hsai_BlockA1, (uint8_t*) buffer, 8, 10);
for (int i = 0; i < 8; i++) {

double sample = buffer[i];
sample = sample * gain; // pre-gain
sample = PeakFilter_Update(&bassBoost, sample);
sample = PeakFilter_Update(&midCut, sample);
sample = PeakFilter_Update(&highBoost, sample);

buffer[i] = (int16_t) sample;
}
HAL_SAI_Transmit(&hsai_BlockA2, (uint8_t*) buffer, 8, 10);
}

EDIT: I'm laughing at myself but this is a classic example.  Classic.  Your code works first time.  "That was easy".  Never pay any attention.  "Some time later...", "That doesn't look right... I'll take a look...."

"Wait.  How does this work?  How did this ever work?  What the hell?"

No... I'm not looking for help to fix it.  I can see a symetry of problems that if fixes will make it make sense, but I just found it funny.  Symmetrical insanity just happens to work.

EDIT2: Oh and it has many other issues to boot.

How do you distinguish between "works" and "broken"?

[Sherlock Holmes]

Note:

Code: [Select]

HAL_SAI_Receive(&hsai_BlockA1, (uint8_t*) buffer, 8, 10);

If that 8 is the size of the buffer to write to, then only 8 bytes are being used despite the fact the declared buffer is 16 bytes long...

The code declares a 16 byte buffer, but it only ever needs and uses an 8 byte buffer, so long as buffer is at least 8 bytes long, everything is fine - is that what you're talking about though?

[thm_w]

Note:

Code: [Select]

HAL_SAI_Receive(&hsai_BlockA1, (uint8_t*) buffer, 8, 10);

If that 8 is the size of the buffer to write to, then only 8 bytes are being used despite the fact the declared buffer is 16 bytes long...

The code declares a 16 byte buffer, but it only ever needs and uses an 8 byte buffer, so long as buffer is at least 8 bytes long, everything is fine - is that what you're talking about though?

16 bytes are used, assuming HAL_SAI_Recieve is set to 16-bit (instead of 8-bit) in the initialization. Which OP is saying it is.

I think the point is that HAL_SAI_Receive is writing to an address using 16-bit each time, but then OP thought the loop was pulling out data 8-bit at a time?
Which I don't think it is, because buffer is a 16-bit array, right?

int16_t buffer = {1, 2, 3, 4};
buffer[0] = 1
buffer[1] = 2

[hans]

That function looks like typical C HAL hackery mess - where 8 defines the number of items and not the number of bytes, and the item size is configured elsewhere. It increments the dataPtr by 2 bytes at a time, and only after byte increments casts it to 16-bits for the write operation. That is functionally correct, but IMO very odd way of dealing with sizes, pointers and casts. But I rest my case, C does not really allow for a type generic implementation without either macros (= bigger mess) or C++ templates..

Anyhow, if we assume that OP tested a SAI passthrough which works and does not discard any samples, then it kind of is a mystery what behaviour is observed. As Sherlock says, what does broken and what does working look like? Are there really zero differences to spot for the 3 different "YES" situations listed?

Considering that the filter behaviour can be tested independently.. one may be able to generate a sweep of input frequencies into a buffer and pull it through the suspecting code. The output samples can then be plotted in e.g. a bode plot. It's a bit tedious work to set up the testbench, and perhaps there are frameworks or tools that don't require you to write everything yourself (e.g. thinking of MATLAB/Octave), but it could give some insight what the numerical operations are affecting to your raw ADC/DAC samples.

However, I think some sort of unit testing is always beneficial for DSP or business logic code like this. The HAL functions serve their purposes of transferring in ADC/DAC data with some real-time element attached to it (not to cause time-related glitches etc.), but other than that the calculations can be modelled very well on a PC with unit tests instead. If that catches 95% of the mistakes before uploading to hardware, it can reduce the head scratching alot between distrusting the HAL configuration/usage and/or your own tested application code.

[SiliconWizard]

That function looks like typical C HAL hackery mess - where 8 defines the number of items and not the number of bytes, and the item size is configured elsewhere.

So? How is a function that takes a number of items as parameter instead of bytes hackery?
The HAL is poorly documented, and that's the real issue (but we all know documentation is hard shit to get right.) When using the HAL, I've always looked at its source code when using HAL functions. Rather than trying to guess. May sound annoying, but that's as good as you'll get.

[helius]

Without refering to the code for HAL_SAI_Receive(), it's not possible to say whether it is correct, or not. I looked at the github page linked above by thm_w, but couldn't understand the workings of the function enough to say. I will note, in passing, that the function is written in an almost unbelievably stupid way, effectively testing whether it is in 8 or 16 bit mode for every single word transferred.

Simply because the code calls that function using a pointer cast to (uint8_t *) doesn't really mean anything. It definitely does not mean that "there are 8 8bit ints". It doesn't even mean that the ints are unsigned! All it does is mollify the type checker that the function is being called with arguments compatible with the function's prototype. The data representation is unchanged, and this cast has no effect on further uses of buffer later in the caller.

I've observed that many C programmers imbue the cast with magical, ineffable powers. For example, the code inside the loop which does

Code: [Select]

buffer[i] = (int16_t) sample;
uses a totally pointless cast expression. When the lvalue and the rvalue in an assignment are both arithmetic types, conversion is automatic.

[thm_w]

I don't disagree that code is not optimized, I wouldn't call it unbelievably stupid though. I doubt the performance difference is significant.

[helius]

I guess it's an opportunity for another gripe which is that hardware companies seldom understand software enough to use it properly. If the prototype for HAL_SAI_Receive() declared that the type of the Data argument was void *, then callers could pass in their 8-bit or 16-bit or 32-bit arrays without any cast at all. Declaring the type as uint8_t* is very misleading since audio samples are always signed!

[SiliconWizard]

I don't disagree that code is not optimized, I wouldn't call it unbelievably stupid though. I doubt the performance difference is significant.

Optimizers almost certainly optimize this by factoring the tests. So while not pretty to read, it probably doesn't make a difference at all, unless maybe you compile with zero optimization.

As to 'uint8_t *' for parameters taking buffers, except if said buffers actually make sense as a succession of 8-bit words, I'm fully against that style. It requires casting if the pointer passed is not actually a 'uint8_t *', and could be misleading indeed regarding any alignment requirement.

Use 'void *', and properly document if there is any alignment requirement or not. No cast needed, no possible confusion.

Note: also, it's pretty straightforward to check alignment at run time from a pointer, and I don't think they do it in the HAL (I would have to check that back.)

For instance, 16-bit aligned:

Code: [Select]

if (((uintptr_t) pointer) % 2 == 0) {... }
Not rocket science.

[westfw]

There are not 8 16bit ints there.  There are 8 8bit ints.

Why do you say that?  The buffer is clearly 8 16bit ints.  So it's all dependent on how HAL_SAI_RECEIVE works.


If HAI_SAI_RECEIVE() "Size" parameter is actually a count of data whose size was specified sometime back in initialization and is stored in the hsai handle data structure.
That appears to be the case.  So it's not your co-workers code that sucks, it's the HAL library (yet again.)

Here's an edited copy of the function (from some STM32F4xx HAL version), with some annotations added.

Code: [Select]

HAL_StatusTypeDef HAL_SAI_Receive(SAI_HandleTypeDef *hsai, uint8_t *pData, uint16_t Size, uint32_t Timeout)
{
   // BLAH BLAH
    hsai->pBuffPtr = pData;
    hsai->XferCount = Size;
   //   BLAH
    /* Receive data */
    while (hsai->XferCount > 0U) {
      if ((hsai->Instance->SR & SAI_xSR_FLVL) != SAI_FIFOSTATUS_EMPTY) {
//***** Pick up "item" size from handle, copy that many bytes
        if ((hsai->Init.DataSize == SAI_DATASIZE_8) && (hsai->Init.CompandingMode == SAI_NOCOMPANDING)) {
          (*hsai->pBuffPtr++) = hsai->Instance->DR;
        } else if (hsai->Init.DataSize <= SAI_DATASIZE_16) {
          *((uint16_t *)hsai->pBuffPtr) = hsai->Instance->DR;
//***** adjust the pointer by the size from the handle.
          hsai->pBuffPtr += 2U;
        } else {
          *((uint32_t *)hsai->pBuffPtr) = hsai->Instance->DR;
          hsai->pBuffPtr += 4U;
        }
//****** Decrement "Size" parameter value by 1
        hsai->XferCount--;
      }
    // More BLAH
    }


[paulca]

It has many issues, not one.  The size of what SAI reads and how the loop goes through it appear to only be part of the problem. 

Anyway.  It shall be rewritten.  First off, the TX should be interrupt, the handling code should be in the ISR.  I shoudn't be calling the SAI HAL function, because I explicitly want to copy the contents of the SAI FIFO only.  I also don't want the FIFOs to block or be consumed if that makes sense, I just want them to free run.  If I read an input SAI at twice the sample frequency I want to receive duplicate FIFO contents.  If HAL decides to go around it's 1ms Tick delay loop it will get ugly.  I also really don't need it to check anything, at this stage all I want it to do really is give me the contents of the Rx FIFO, verbatim as 2x32bit ints.  I'll do the rest.  That should be a simple read and shift the 8 bytes out of the hardware array.  No need for HAL abstractions for that part.  I have a limited cycle budget obviously.

I just found it funny that it worked, like I wrote it out in more or less one go (i've played a lot with the eq params), ran it and it played music with EQ without distortion.  I've been using it on trial for a week now.  It was only when I read what I'd written I went, "Wait... hang on."

[paulca]

I've observed that many C programmers imbue the cast with magical, ineffable powers. For example, the code inside the loop which does

Code: [Select]

buffer[i] = (int16_t) sample;
uses a totally pointless cast expression. When the lvalue and the rvalue in an assignment are both arithmetic types, conversion is automatic.

I believe the term used is "explicit cast" where an "implicit cast" would have occurred anyway.  There is a reason why it exists and even has a name for the technique.

[Siwastaja]

Being explicit over implicit is not "totally pointless"; code is written to communicate the intent to both human readers, and compilers.

I really struggle to see why most libraries are so poorly documented. I mean, given typical 3-4 arguments for a function, it takes maybe 5-10 minutes to write a simple comment which lists all the arguments and clearly explains what they are. Heck, I consider myself a lazy documentation writer yet my own internal functions are usually better documented than ones in widely used libraries.

There should never be confusion if a parameter is a count or size. Count should be named count, and size named size, and then the comment should read:
// number of items
or
// size in bytes

It's sad such a thing even needs to be discussed, but result of absolute laziness during code writing wastes a lot of time afterwards. Why don't standards like MISRA C mandate commenting the purpose and usage of each argument and return value of non-static functions?

[paulca]

Being explicit over implicit is not "totally pointless"; code is written to communicate the intent to both human readers, and compilers.

Indeed.  It can also catch some fairly nasty surprise bugs should someone want to attempt changing the type of the receiver variable to see what happens.  Probably not a big concern in a single C function, but good habits are habits.

[westfw]

First off, the TX should be interrupt, the handling code should be in the ISR.  I shoudn't be calling the SAI HAL function

Ah, but those are "failures of design" rather than "failures of coding."

[Leeima]

I guess it's an opportunity for another gripe which is that hardware companies seldom understand software enough to use it properly.

In my experience I've noticed the inverse is often true! Software companies seldom understand hardware enough to use it properly! 

[helius]

From The Hacker's Dictionary:

finger-pointing syndrome: n.

All-too-frequent result of bugs, esp. in new or experimental configurations. The hardware vendor points a finger at the software. The software vendor points a finger at the hardware. All the poor users get is the finger.

[paulca]

Well, this is my second attempt at the same:

Caveats:  It's hardcoded for 1 Rx 1 Tx for now and fresh out of the fingers.

Code: [Select]

uint32_t rx1Buffer[2];
uint32_t tx1Buffer[2];

void SAI_Tx_Interrupt(SAI_HandleTypeDef *hsai) {
// Get rx1 it status
uint32_t ite = NVIC_GetEnableIRQ(SAI1_IRQn);
// Shhh it
NVIC_DisableIRQ(SAI1_IRQn);
// Move its FIFO to Tx buffer
tx1Buffer[0] = rx1Buffer[0];
tx1Buffer[1] = rx1Buffer[1];
        // Release it
if (ite) {
NVIC_EnableIRQ(SAI1_IRQn);
}
        EQ_Run(tx1Buffer);
////////////////////////////
/* Transmit 2x32 bits of data */
hsai->Instance->DR = tx1Buffer[0];
hsai->Instance->DR = tx1Buffer[1];
}

void SAI_Rx_Interrupt(SAI_HandleTypeDef *hsai) {

// Dont want the Tx reading as we write
        uint32_t ite = NVIC_GetEnableIRQ(SAI2_IRQn);
NVIC_DisableIRQ(SAI2_IRQn);
////////////////////////////
/* Receive 2x32 bits of data */
rx1Buffer[0] = hsai->Instance->DR;
rx1Buffer[1] = hsai->Instance->DR;
if (ite) {
NVIC_EnableIRQ(SAI2_IRQn);
}
}

Much less code.  A basic "borrow and strip" approach from HAL in most cases.  "Works" in that it through-puts music with EQ.  I have noticed one or two clicks, so it might need a bit of tweaking.

The handling of the actual 16bit (in this case) audio, now becomes much cleared.

The raw 32bit words contain right hand justified signed 16bit words.

Code: [Select]

void EQ_Run(uint32_t*buf) {
int16_t *buffer = (int16_t*)buf;
for (int i = 0; i < 4; i+=2) {
double sample = buffer[i];

Now that's a bit clearer, I can try 24bit.

EDIT:  I'm still a little concerned about:
   for (int i = 0; i < 4; i+=2) {

Which could be "left" justified surely?

[Siwastaja]

Geez, if you want to control how interrupts pre-empt each other, just set the priorities accordingly (NVIC_SetPriority(IRQn, priority)) once. Enabling and disabling other interrupt sources from other ISRs is an ugly hack, there is absolutely no need for this kind of crap, it very likely has some nasty corner cases.

[paulca]

Geez, if you want to control how interrupts pre-empt each other, just set the priorities accordingly (NVIC_SetPriority(IRQn, priority)) once. Enabling and disabling other interrupt sources from other ISRs is an ugly hack, there is absolutely no need for this kind of crap, it very likely has some nasty corner cases.

I know.  Giving them equal prio and equal sub-prio and they shouldn't interrupt each other.

But....  the thought I had, was, when there are more than one Rx SAI, I wanted to limit the granularity of the interrupts, not disable them entirely.  EG:

Silence Rx1
   Copy Rx1 buffer
Release Rx1
Silence Rx2
  Copy Rx2 buffer
Release Rx2
Silence Rx3
  Copy Rx3 buffer
Release Rx3

Might be over kill / unnecessary... or might make things wose!

[paulca]

Good news, bad news.

Good news, it works and after some tweaking and testing, it's verified as working.

Bad news.  1 stereo input + 6 peak filters = 69% duty cycle.  Actually that makes it sound better than it is, as many things upset it skipping into half sample rating the output to 24k.  That's 48k 16bit too.  Dreams of 96k and 24bit are dashed entirely.

I can optimize, but I'm not sure I'm going to make the following filter code at least twice as fast.??

Code: [Select]

float PeakFilter_Update(PeakFilter_t *filt, float in) {
filt->x[2] = filt->x[1];
filt->x[1] = filt->x[0];
filt->x[0] = in;

filt->y[2] = filt->y[1];
filt->y[1] = filt->y[0];

filt->y[0] = (filt->a[0] * filt->x[0] + filt->a[1] * filt->x[1]
+ filt->a[2] * filt->x[2]
+ (filt->b[1] * filt->y[1] + filt->b[2] * filt->y[2])) * filt->b[0];
return (filt->y[0]);
}


[paulca]

Caps showing the Tx and Rx interrupt duty cycles. 

First with 2 samples.
Second with 8 samples.

As an aside.  I had a rather unpleasant practical in Nyquist aliasing and artefacts when I tripple checked the 24kHz output sample rate, but.... playing a 20kHz sine wave through it.  My recommendation is... Do not try this at home.  It's unpleasant. I don't think the PCM5102's filterless output filter liked it.  As best I could determine on an FFT it was warbing between 3.8 and 4.2 kHz!

[paulca]

It would seem I under-estimated just how well the code optimizes.  -Ofast and -O3 both take it from a 67% ish DC to about 21%.  Impressive.

That changes the outlook towards the positive.  I've already spoken too soon on this bit of code about 3 times now, but ... a basic test, non conclusive, suggests I might squeeze a 5 Band EQ + a 3 Band EQ and dual outputs and at 96kHz.

Testing should reveal.  I expect it might be tight, but in theory it can be tight and still work if interrupts are under control.

[cv007]

The use of unions/structs can also be easier to use than casting, especially when done on a single type in various places. The casting gets eliminated and each use will be easier to understand (your 'casting' equivalent takes place only once when you create the union/struct). There are many ways one could go about this, but the following is one example-

Code: [Select]

static const uint32_t SAI_BUF_SIZE = 2;
typedef union {
    volatile uint32_t dr_u32;
    volatile int16_t eq_i16;
} sai_buffer_t;

sai_buffer_t rx1Buffer[SAI_BUF_SIZE];
sai_buffer_t tx1Buffer[SAI_BUF_SIZE];

//usage

//maybe always 2, don't know, but can use loop in any case as optimizer will figure it out and produce optimal code
for(int i = 0; i < SAI_BUF_SIZE; i++ ){
        tx1Buffer[i].dr_u32 = rx1Buffer[i].dr_u32;
}


EQ_Run( tx1Buffer );

void EQ_Run(sai_buffer_t* buf) {
for (int i = 0; i < SAI_BUF_SIZE; i++) {
double sample = buf[i].eq_i16;
                ...



Obviously lots of options and whether this one is better or not, I like to eliminate casting if there is a better option. In this case I would rather deal with the union than to have to (re)think about how the underlying data fits together when casting.