midilab / uClock

A tight BPM clock generator for Arduino and PlatformIO using hardware timer interruption. AVR, Teensy, STM32xx, ESP32 and RP2040 support
https://midilab.co/umodular
MIT License
156 stars 20 forks source link

Possible to use concurrently with the USB_Host_Shield_2.0 library? #4

Closed doctea closed 2 years ago

doctea commented 2 years ago

Hi,

I'm trying to use the uClock library with the USB Host Shield library (https://github.com/YuuichiAkagawa/Arduino-UHS2MIDI and https://github.com/felis/USB_Host_Shield_2.0). I want to use it to sync clocks with multiple devices and CV clock.

I've got a sketch mostly working for this purpose but wanted to give it more rock-solid timing so attempted to implement uClock with it, available here: https://github.com/doctea/usb_midi_clocker/tree/uclock

However, having added uClock, it seems to stop the USB Host library from working and detecting devices. I imagine perhaps because of some interrupt conflict or suchlike? It seems like the Arduino crashes and restarts when the two are enabled together.

Its a bit beyond me to work out how to debug and fix this -- are there any pointers I should start with and is there something that can be done to make the two libraries coexist happily?

Thanks,

midilab commented 2 years ago

Im not aware of how this library works internally, but uClock make use of timer1, in case this library uses timer1 or any of his dependencies uses it too, it wont work.

There is a code block at uclockInitTimer() on uClock.cpp, where you find one for timer1 and other commented for timer2. You can try to make use of timer2 instead by commenting the timer1 code block and uncomment timer2 code block.

But you need to check if that library make uses of any other timer, using timer2 can also affect other internal arduino libraries that make use of it, you can search over the net for what functions will make use of timer2, i'd never tried to make use of it.

Any other than that you need to debug your code to check for possible errors at the implementation.

doctea commented 2 years ago

Thankyou, I'll try that!

doctea commented 2 years ago

Thanks again, upon further testing it does seem that the problem was probably just that my sketch was too large and didn't leave enough memory and that was what was causing the crash whenever a USB device was being detected! Thanks for your help and work on your software :)

doctea commented 2 years ago

Hmm, I think maybe I spoke too soon about it working OK. I've spent all day trying to get the two working nicely together, but am still running into strange problems (USB stops receiving data if tempo is changed after starting sketch; tempo is wrong/very slow if not changed after starting...). Switching to TIMER2 in uClock seems to lead to neither working at all. Maybe it is something simple, but I'm definitely stuck at the moment!

midilab commented 2 years ago

well i will check asap the timer2 config, i think i never tested, probrably there are some trouble with the initialize of timer2.

but it worth a check from your side to confirm if the library you're using make use of any arduino timer, directly ou indirectly.

doctea commented 2 years ago

Thanks @midilab! I checked it as far as I know how to do (grepping for 'timer' and 'ISR' in the library folders), but I'm also not sure which other libraries it relies on so I still might be missing something.

I've gone back to using a non-interrupt method of keeping time, and restructured my code so that its running acceptably for me now, so it isn't such a pressing problem for what I'm doing right now. But it would be cool if I could be sure I was using more solid timing, so thanks if you do get a chance to look at it :)

midilab commented 2 years ago

So it happens that the initialization of timer2 was wrong. I had fixed and add a #define to chose a timer to make use.

So now you can test it by comnent //#define AVR_TIMER_1 and uncomment

define AVR_TIMER_2

at your uClock.cpp

Let me know if it works! in case doesn't work at all, your code needs to be checked.

midilab commented 2 years ago

this is the new release: https://github.com/midilab/uClock/releases/tag/0.10.6

doctea commented 2 years ago

Thank you for your time looking at this. I think with this new version that there is some improvement in how it is working -- now, the USB devices are detected, input is received by the sketch and output received by USB devices, and uClock keeps ticking (albeit slowly). (Previously there would be strange crashes and things would not work at all, so I think this is improvement).

Now, when my sketch is powered on without any USB devices attached, uClock ticks correctly and my sketch behaves as expected, at the tempo as expected. However, as soon as I plug in a USB device, the uClock-driven tempo drops. Oddly, the more devices I connect, the slower it seems to get. If I unplug the USB devices, the uClock tempo goes back to normal.

I've experimented with disabling parts of my code to see if this affects it, but haven't found anything yet that gives any clues to what's going on. Due to the nature of the sketch and the shields attached and this Arduino's refusal to talk over serial monitor it isn't easy for me to debug quickly.

The only other thing that gives me pause is that I get the warning "Global variables use 1565 bytes (76%) of dynamic memory, leaving 483 bytes for local variables. Maximum is 2048 bytes. Low memory available, stability problems may occur." when compiling, so perhaps that is having some detrimental effect too -- presumably connecting a USB device is eating up more memory also.

Either way, thank you for your help so far, I will try experimenting with this some more and will post back any findings!

midilab commented 2 years ago

well, no improvements, only a timer initialize fix and add of a new timer0 option.

but as you saying looks like your code is in trouble, remind that any variable shared between interrupted calls and non interrupetd calls need to be carefully managed(volatile, stop interrupt to avoid concurrency problems... and so on...), also interrupted calls needs to be as short as they can, specialy the uClock one! remeber that cpu power is not infinite, neither memory.

stack to heap collision is quite common on embbeded low memory programming, so take care of it by monitoring your space in between then.

i use this function to monitor my memory space between heap and stack when programming. you always need some bytes in between to the movement of stack around, or heap.

int freeRam ()
{
  extern int __heap_start, *__brkval;
  int v;
  return (int) &v - (__brkval == 0 ? (int) &__heap_start : (int) __brkval);
}

I can't say much without looking at your code, but those listed things i've made here are quite know causes to abnormal behaviour of a embbed program, specially those with interrupted concurrency.

midilab commented 2 years ago

probrably the new rewrited engine can help you out: https://github.com/midilab/uClock/tree/v1

its on the branch called v1, i had finish the rewrite and some basic tests, now the uClock gives you much more room to code between interrupted calls to avoid lose timming. its a huge performance increment! try out and let me know if it works for you.

any way, copy the code you're using with uClock callbacks here for me to be able to help.

doctea commented 2 years ago

Thanks once again for your help and work on this @midilab !

I've used the new version you mention, and spent more time trying restructuring my code, trying to pinpoint what causes crashes, and adding or removing ATOMIC()s, and I seem to be making some progress. Currently my sketch has been running for several minutes without crashing, which is a new record! Fingers crossed I'm getting somewhere with it.

My code and the prototype uClock branch is over at https://github.com/doctea/usb_midi_clocker/tree/uclock -- it packs quite a lot of functionality in and the code is not the cleanest it could be, so I don't expect you to read it all, but if you are interested to see what I'm trying to do then please do take a look, maybe you will be able to tell straight away where I have been going wrong.

Otherwise, I will report back with my progress. Thanks again =)

midilab commented 2 years ago

restart_on_next_bar and ticks are the variables you need to take care of make then volatile, ATOMIC() should happen on a non-interruptet running code trying to access a variable that changes value inside a interrupted one. So ATOMIC() stops all interruption until it finishes to make sure the variable are not being read and updated on non atomic operation(by interrupted and non interrupted code).

Also, try to process the min you can on interrupts and use volatile variables to set states for later hard process on non-interrupt context( loop() ), try to get to non-interrupted all non-realtime related code.

There is a sequencer i wroted some years ago, minincs the 303 step sequencer. maybe you can take a look at to get some insights: https://github.com/midilab/aciduino/blob/master/Aciduino/AcidStepSequencer.ino

https://www.youtube.com/watch?v=PR0pkCa_ahI

doctea commented 2 years ago

I said I would report back -- and the latest build has been running for 4 hours now without any crash! The trick seemed to be that I needed to make sending of MIDI commands over USB ATOMIC(), and as you say, to be sure to avoid using ATOMIC() inside functions that are called from inside interrupts, and to make time spent in interrupts as short as possible.

It still crashes when I enable the extra 'sequencer' functionality, so I need to look at this again (altho I am also getting warnings about low memory, so that could be the problem -- thank you again for the function that helps to keep an eye on this, I'll implement that in the next version of my code so that I can identify if that is causing the problem a me.)

And yet again, thank you so much for your help with this, and for looking over my code! Thank you for those tips, I will try to implement what you suggest.

I did previously have a look at your example sequencers to try and figure out what I was doing wrong and how I should structure things, but I already had all my functionality working otherwise without uClock and so did not want to have to completely restructure it all :). The example definitely gave me some clues about what was acceptable or not to use inside and outside interrupts.

doctea commented 2 years ago

Unfortunately my sketch still seems to be very unstable, with the main loop randomly crashing if one of the USB MIDI devices is receiving CC messages -- may need to rethink my approach, suspect its because the CC handling function/loop takes too long.

midilab commented 2 years ago

I remeaber to had a lot of headshake with serial stuffs and interruptions.

you also need to make sure that no code is trying to use USB or any other serial outside interruption without ATOMIC(in case there are some code using Serial/USB inside interruption).

So serial interfaces(USB too) communications also needs to be ATOMIC operations.

I see a lot of Serial.print() on your code, for interrupted and not interrupted... do never attempt do do read or write on a serial(USB too) outside interruptions without ATOMIC if you are using it inside a interrupted call too.

Also a function parameter doesn't need to be volatile, volalile only the variables that are shared between interrupted and non interrupted context.

midilab commented 2 years ago

Investiganting another issue related to USB midi and uclock on another platform i check the code for the usage of timers, and in fact the library make use of timer.

So maybe the problem is related to timer usage conflict(uclock needs the timer alone for him self).

The good news is that the guys that code that library leave a option definition to setup different timer for use.

In your case you can try to

define USE_TCCR2A 1

to force USB library to use timer2 instead of uClock's timer1

Im not sure if arduino compiles the code in a way that a .ino definition like that could affect the library scope... any way, the worst case you can make a hardcode definition inside the file that uses it. avrpins.h

Let me know if that solves the issues