[gruvin9x] Mixer running in an interrupt

[GoogleCodeExporter]

Having the Mixer running in an interrupt was an easy solution for allowing the 
SD Logs function on gruvin9x board.

Latency has been measured by Bryan and is very good.

But after having reviewed the code and discussed with Fox and Romolo about 
issue 157 (relative to stock board), I think we need to find another solution 
ASAP.

There are 2 big issues with this implementation:

1) Since I have enabled the long names support on SD, the available RAM has 
decreased a lot, and we run out of stack too quickly... Even if I still think 
that this problem can be solved short term, it will prevent us from adding new 
functionality.

2) I have added a lot of couples pauseMixerCalculations / 
resumeMixerCalculations to avoid race conditions. Especially for 16bit 
variables written in the main (menus) and read in the mixer (interrupt). But it 
remains a lot, written by the mixer and read by the main (g_chans512, 
calibratedStick, trims, ex_chans, rotenc positions...).

Original issue reported on code.google.com by bson...@gmail.com on 20 Nov 2012 at 8:42

[GoogleCodeExporter]

Original comment by bson...@gmail.com on 20 Nov 2012 at 8:43

[GoogleCodeExporter]

Copy/Paste of Fox comment in issue 157:
---------------------------------------

Another issue, I forgot to mention. If SD card is installed, the original way 
is much more slower. Since the implementation of writeLogs() is improper.

For such a low-end platform, we should avoid SPI i/o with SD card.
Suggested procedures as follows:

1. Open file

    // Pre-allocate FAT cluster for speedy write
    filesize = File1.fsize;

    if (filesize < 102400) {
        res = f_lseek(&File1, 102400);
        res = f_sync(&File1);

        f_lseek(&File1, 0);
        filesize = 102400;
    }

    // Seeking to first unused record
        ...

The point is, SD card is large enough (e.g. 8GB or etc).
We allocate a bigger file, e.g. 100K, and sync, this make FAT updated and no 
more access is necessary for next 100K bytes written.

Then, just do f_write!

each f_write only modify buffer in the RAM.

If buffer is full, it will be flushed into SD, only 512bytes write is involved.
Normally only 1 SPI transaction involved. Addressing time of SD is limited.

No addition FAT load / modification / write involved.

Another issue is too many xx_printf() used. 
printf() involved lots of conditional test & string manipulate, is very very 
slow.

Original comment by bson...@gmail.com on 20 Nov 2012 at 8:46

[GoogleCodeExporter]

The solution I have in mind today :
-----------------------------------

(being said that I often change my mind)

We can't re-code the FatFS layer to be ASYNC instead of SYNC. I did it for 
EEPROM, it was not simple, I introduced many regressions despite it was MUCH 
more simple than FatFS!

So an easy solution, if we want to have the mixer running in the main (as done 
on stock board after patch inside issue 157 is applied) is to use another 
function in diskio.cpp than loop_until_bit_is_set(...). The new function would:
1) loop (of course!)
2) check for nextMixerTime, if getTmr16KHz() > nextMixerTime, it calls 
doMixerCalculations()

This way should give us the same good results than the interrupt way I used, 
but with far less problems!

Original comment by bson...@gmail.com on 20 Nov 2012 at 9:00

[GoogleCodeExporter]

And as Fox writes above, most of f_printf(...) should be removed from logs.cpp, 
it was a quick way to achieve what we wanted, but by far not the most 
efficient! It will be done in a second step.

Original comment by bson...@gmail.com on 20 Nov 2012 at 9:03

[GoogleCodeExporter]

>There are 2 big issues with this implementation

Another big issue comes from integrating others' code 
e.g., for telemetry / voice / new hardware / game or etc., ^_^
It is common to run out of stack.

We must review all new code in the future to ensure them NOT be a stack monster.
(All local variables must be examined)

---

I agree Comment 3 is more easy, can be a start.

but take care of LCD redraw, and re-entrance.

Original comment by myfo...@gmail.com on 20 Nov 2012 at 9:12

[GoogleCodeExporter]

No re-entrance problem there, it's impossible to write anything to SD / EEPROM 
/ LCD ... from doMixerCalculations() which should only do calculations, that's 
the rule!

Original comment by bson...@gmail.com on 20 Nov 2012 at 9:29

[GoogleCodeExporter]

Agree, "only calculations" ! 
the rule is clear and easy to obey.

Original comment by myfo...@gmail.com on 20 Nov 2012 at 9:49

[GoogleCodeExporter]

Then, LCD redraw should not be a problem unless loop_until_bit_is_set is 
extreme long.
(SD access timeout?)

Original comment by myfo...@gmail.com on 20 Nov 2012 at 9:52

[GoogleCodeExporter]

Which is already the case today, as LCD and SD Logs are within the main, so run 
in sequential order.

Original comment by bson...@gmail.com on 20 Nov 2012 at 9:54

[GoogleCodeExporter]

No, I meant blocking within the waiting SD loop for 1s. 

(only example, check if filesystem has timeout mechanism and/or SPI under layer 
access timeout)

Original comment by myfo...@gmail.com on 20 Nov 2012 at 10:01

[GoogleCodeExporter]

Original comment by bson...@gmail.com on 20 Nov 2012 at 10:34

• Changed state: Fixed

[GoogleCodeExporter]

I've only just caught up on all this. To me, the "solution" is simple (I don't 
understand yet what has actually been done, for what it's worth). My view is 
that we are producing first and foremost an RC controller. Therefore, mixer 
calcs and data output and all things directly related to that are of the utmost 
priority and everything else can just wait.

If that means that a file doesn't get written exactly when we wanted it too or 
the LCD display misses an update cycles then TOO BAD!No? I absolutely cannot 
understand any need for "pauseMixerCalcs". That should NEVER happen.

Anyway, this is flagged fixed, so I suppose you all must have done something 
clever to resolve the issue I don't even understand. (Doh!) But I do hope 
there's no pauseMixerCalcs anywhere now. (And I'm probably missing some key 
point, as usual ... and I'm sweating and coughing as I type, with high stupid 
@#%@! fever. But hey. :-P)

Original comment by gru...@gmail.com on 22 Nov 2012 at 6:32

[GoogleCodeExporter]

Oh yes and -- yeah. that's why I never did long filename support in the first 
place. In that case, it was because I understood that a "code page" was 
required and that took up lots of, well, everything. That was also why my 
originally suggested file naming scheme for model backup/restore was a bit 
cryptic, allowing the main part to spill over into the extension and requiring 
that all models be stored in a specific directory -- as they still are 
anyway.Noting that then, I say we don't need long filenames at all and should 
remove it, in favour of the above.

Original comment by gru...@gmail.com on 22 Nov 2012 at 6:35

[GoogleCodeExporter]

Having the mixer running in an interrupt does cause some problems, for example:

1) we modify the CHx offset (16bits) in the menus. It's possible that the 1byte 
is written and the interrupt happens, before the 2nd byte. The used value 
inside the mixer will be completely wrong!

2) we remove a mix, a dual rate

3) we move a trim (from the mixer) and we draw it on screen in the main view 
menu

So ... too many race conditions that make the code too difficult to maintain, 
having cli() / sei() everywhere is just something I don't want. 

The solution we implemented with Fox should achieve exactly the same results 
but without the interrupt. Perhaps you could have a look to the description in 
issue 157?

Original comment by bson...@gmail.com on 22 Nov 2012 at 7:01

[GoogleCodeExporter]

Short description of the new implementation: We run the menus when we are sure 
that we don't need to run the mixer

Original comment by bson...@gmail.com on 22 Nov 2012 at 7:02

[GoogleCodeExporter]

pauseMixerCalcs only happens on the previous implementation. 

When issue160 got fixed, all pauseMixerCalcs are removed!

Thanks bsongis for trying everything to remove them.

Original comment by myfo...@gmail.com on 22 Nov 2012 at 7:21

[GoogleCodeExporter]

Oh. Now I see where the race conditions are. It's obvious, now you mention it. 
Thank you. Actually, I was only thinking about SD card access, since that was 
the discussion above. Couldn't see any chance of race conditions there. :-P

Clearly, I haven't seen Issue 157, yet. Off to read up on that now.

Original comment by gru...@gmail.com on 22 Nov 2012 at 10:50