Midi Out USART in C with atmega328p

Go To Last Post
14 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hello All! There are a few good discussions on this topic already but none seem to be complete. I'm hoping with some help from the community, I can figure out what is not working with my implementation and generate some useful resources for others working with midi while I'm at it. 

 

HARDWARE/PROGRAMMING TOOLS

Arduino Uno (Atmega328p)

Midi Out DIN wired to these specs: https://learn.sparkfun.com/tutor...

USBtinyISP serial programmer

AVRdude

Atmel studio 7

 

WHAT HAS ALREADY WORKED

This program is able to toggle an LED via a button push. The next step is to send a midi clock tick when pushing the button. As a starting point for USART, I set up the micro controller to send a char over serial using the built in usb port on the arduino. I am using Tera Term to monitor the serial COMM port and have been able to send characters only, no int and hex values are interpreted as ASCII. 

 

MIDI

For the midi setup, there is one start bit (low), followed by a byte, followed by a stop bit (high), no parity bit used. Most midi messages require more than one byte of data but midi timing clock only requires one byte which is why I am choosing it as a starting point. The code below should send one clock tick via DIN midi out port each time the button is pressed but it does not work. I am using midiOX as a console utility to verify outgoing messages. I can send data from a midi controller through a midi interface and monitor the incoming data in midiOX. I connected the midi interface to the arduino via DIN so if I am able to successfully send a midi message out of the Arduino the data will show up in midiOX.  

 

CODE

This code compiles but does not send the desired midi message. The code be very close to doing what it's supposed to but there is something that we are just not seeing. Hoping someone else might catch it. The solution to this has been evading my collaborator and I for a while and any help would be greatly appreciated!

 

#ifndef F_CPU
#define F_CPU 16000000UL // 16 MH clock speed 
#define BAUDRATE 31250 // BAUD rate for Midi
//#define BAUDRATE 9600 // BAUD rate for Serial
#define BRR (((F_CPU / (BAUDRATE * 16UL))) - 1) // BAUD rate register
#endif

#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>
#include <string.h>

int cliFlag = 0;    //variable used to enable and disable interrupts

char clock_tick = 0b11111000; // binary for timing tick

// transmit midi out
void midiTransmit(char m) // USART transmit function based on polling of the data register empty (UDREn)
                          //flag. When using frames with less than eight bits, the most significant bits written to the UDRn are ignored.
{
    while (!(UCSR0A & (1<<UDRE0))); {} //wait for UDR0 to be empty
    UDR0 = m; //send value 
}

int main(void)
{    
    DDRB = (1 << PORTB5); //PORTB5 as output - this is for LED on PIN13
    PORTB = (1 << PORTB5); //LED starts in ON state
    
    PORTD = (1 << PORTD3); //enable pull-up resistor and INT1 PIN
    
    EIMSK = (1 << INT1); //enables INT1 interrupt in External Interrupt Mask Register
    EICRA = 0x08; //enables falling edge as interrupt request by setting ISC11 to 1 
                    //for more information refer to section 12.2.1 in Atmega328p data sheet

    // MIDI OUT AND SERIAL COMMUNICATION
    UBRR0H = (BRR >> 8); // Upper Register (only 1 nibble used)
    UBRR0L = BRR; // Lower Register (full byte)
    
    UCSR0B = (1 << TXEN0); // transmitter enable for midi out
    UCSR0C = (1 << UCSZ01) | (1 << UCSZ00); // select 8-bit character size, start and stop bit defaults are correct, asynchronous

    while (1) //to infinity and beyond!
    {
        if(cliFlag == 0)
        {
            sei();    //enable global interrupts when cliFlag is cleared
        }
        else
        {
            cli();    //disable global interrupts
            midiTransmit(clock_tick); // send clock tick data        
            cliFlag = 0;    //clear flag
        }
    }
}

ISR(INT1_vect)    //Interrupt Service Routine - used to toggle LED when button is pressed
{
    
    PORTB ^= (1 << PORTB5);    //toggle LED
    cliFlag = 1;    //set flag so ISR is disabled once returned to main 
    
}

 

 

 

Last Edited: Wed. Sep 16, 2020 - 05:39 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

a couple of small things....

 

first best to put your code in a code block that is the symbol  "<>" that makes it standout from the text and thus makes it better readable.

 

second you can just do

UBRR0 = BRR;

third

int cliFlag = 0;    //variable used to enable and disable interrupts

This flag should be "volatile" as it is used by both the main program and the interrupt service routine.

 

Now in respect to that flag... You have chosen to disable global (so all) interrupts to prevent the interrupt from occurring more than once ( as you probably have button bouncing and thus had more than 1 interrupt )

That is a very dangerous thing to do. Here there is only 1 interrupt, but if you would have also had a uart interrupt active that also would have stopped working. Best is to only disable the interrupt you want to disable.

 

Also in general, keep in mind that you are working with an 8 bit processor. No need to make the flag a integer, just make it "uint8_t" .

 

Rereading the code makes me wonder about it at all as it makes absolutely no sense.

Just remove the cli() and sei().... if the flag is set in the interrupt you transmit a byte ( first check if buffer empty then send) if the flag is not set no need to do anything other than check again...

If you get a second interrupt before the first byte is done. The wait for buffer empty loop will ensure it will wait until the next byte can be transmitted....... Keep in mind that the cli en sei instructions do not fundamentally change this flow as after the first byte is put in the buffer immediately the interrupts are enabled again.

 

Now back tot he main question.... my guess is that because of the not being volatile of the flag bit does not give a change of this bit in the main routine. As in the main code the flag never changes the byte is never send.

 

Change your code to:

 while (1) //to infinity and beyond!
    {
        if(cliFlag == 1)
        {
// interrput has occurred need to
            midiTransmit(clock_tick); // send clock tick data        
            cliFlag = 0;    //clear flag for next button push
        }
    }

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thanks for the informative response, meslomp! I'm going to think about your comments and adapt my code with your suggestions. I will post the results here and let you know how it goes. 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

another small thing

nullspace-voyager wrote:

int cliFlag = 0;    //variable used to enable and disable interrupts

As well as saying that it is used, the comment should also say how it achieves that; eg,

volatile uint8_t cliFlag = 0; // Set to 1 to enable interrupts; 0 to disable interrupts.

The name could also be better chosen as a direct statement of its function; eg,

volatile uint8_t interruptsEnabled = 0; // Set to 1 to enable interrupts; 0 to disable interrupts.

Then you can write, for example;

if( interruptsEnabled )

and the code reads exactly as it does.

 

 

Top Tips:

  1. How to properly post source code - see: https://www.avrfreaks.net/comment... - also how to properly include images/pictures
  2. "Garbage" characters on a serial terminal are (almost?) invariably due to wrong baud rate - see: https://learn.sparkfun.com/tutorials/serial-communication
  3. Wrong baud rate is usually due to not running at the speed you thought; check by blinking a LED to see if you get the speed you expected
  4. Difference between a crystal, and a crystal oscillatorhttps://www.avrfreaks.net/comment...
  5. When your question is resolved, mark the solution: https://www.avrfreaks.net/comment...
  6. Beginner's "Getting Started" tips: https://www.avrfreaks.net/comment...
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

If the volatile thing doesn't fix it the next thing to look for is the timing of things. Your 31250 baud rate calculation is dependent on the fact that  you are saying the AVR is running at 16MHz - but is it really? How have you proven that? If, for example you use a scope on the TXD pin what pulse timing do you see (to check this it will be better to temporarily change the clock_tick value from 0b11111000 to be 0b01010101)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:

WHAT HAS ALREADY WORKED

This program is able to toggle an LED via a button push. The next step is to send a midi clock tick when pushing the button. As a starting point for USART, I set up the micro controller to send a char over serial using the built in usb port on the arduino. I am using Tera Term to monitor the serial COMM port and have been able to send characters only, no int and hex values are interpreted as ASCII. 

 

thought at that too, but the OP stated:

nullspace-voyager wrote:

WHAT HAS ALREADY WORKED

This program is able to toggle an LED via a button push. The next step is to send a midi clock tick when pushing the button. As a starting point for USART, I set up the micro controller to send a char over serial using the built in usb port on the arduino. I am using Tera Term to monitor the serial COMM port and have been able to send characters only, no int and hex values are interpreted as ASCII. 

 

So that suggests that he has had successful serial communication and thus knows the actual frequency the CPU is running on.

 

re-reading the code:

#ifndef F_CPU
#define F_CPU 16000000UL // 16 MH clock speed 
#define BAUDRATE 31250 // BAUD rate for Midi
//#define BAUDRATE 9600 // BAUD rate for Serial
#define BRR (((F_CPU / (BAUDRATE * 16UL))) - 1) // BAUD rate register
#endif

there should not be a #Ifndef .... #endif here.

If somewere else a faulty number is given there for sure will not be a BRR calculation.......

But then again if the baudrate were off I would expect that the OP would have tolled us there is only garbage coming out ( or actually there is and the terminal program rejects it) .....

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you all for the feedback. I will try to address everything in this comment.

 

awneil, great tips for better code styling! I have incorporated your suggestions into my code. 

 

clawson wrote:

If the volatile thing doesn't fix it the next thing to look for is the timing of things. Your 31250 baud rate calculation is dependent on the fact that  you are saying the AVR is running at 16MHz - but is it really? How have you proven that? If, for example you use a scope on the TXD pin what pulse timing do you see (to check this it will be better to temporarily change the clock_tick value from 0b11111000 to be 0b01010101)

 

clawson, I have not verified with a scope that the clock is running at 16MHz. As meslomp noted, I was able to send over serial using 16MHz so that was my evidence that the clock speed is correct. I have also tried running the code in my original post using an 8MHz clock speed as a test and it did not work. My collaborator has a scope and I will ask him to double check. 

 

Here is the code again but with the changes from this thread. It does not toggle the LED anymore on button press and isn't sending the midi message unfortunately. My collaborator wrote the button press and interrupt code and I wrote the serial/ midi communication code. I'm new to programming micro controllers so I need to spend a little time reading about how the interrupts work to understand how removing the cli() and sei() functions affects the code. 

 

#define F_CPU 16000000UL // 16 MH clock speed 
#define BAUDRATE 31250 // BAUD rate for Midi
//#define BAUDRATE 9600 // BAUD rate for Serial
#define BRR (((F_CPU / (BAUDRATE * 16UL))) - 1) // BAUD rate register

#include <avr/io.h>
#include <avr/interrupt.h>
#include <util/delay.h>
#include <string.h>

volatile uint8_t interruptsEnabled = 0; // Set to 1 to enable interrupts; 0 to disable interrupts.

uint8_t clock_tick = 0b11111000; // binary for timing tick

// transmit midi out
void midiTransmit(uint8_t midiByte) // USART transmit function based on polling of the data register empty (UDREn)
                          //flag. When using frames with less than eight bits, the most significant bits written to the UDRn are ignored.
{
    while (!(UCSR0A & (1<<UDRE0))); {} //wait for UDR0 to be empty
    UDR0 = midiByte; //send value 
}

int main(void)
{    
    DDRB = (1 << PORTB5); //PORTB5 as output - this is for LED on PIN13
    PORTB = (1 << PORTB5); //LED starts in ON state
    
    PORTD = (1 << PORTD3); //enable pull-up resistor and INT1 PIN
    
    EIMSK = (1 << INT1); //enables INT1 interrupt in External Interrupt Mask Register
    EICRA = 0x08; //enables falling edge as interrupt request by setting ISC11 to 1 
                    //for more information refer to section 12.2.1 in Atmega328p data sheet

    // MIDI OUT AND SERIAL COMMUNICATION
    //UBRR0H = (BRR >> 8); // Upper Register (only 1 nibble used)
    //UBRR0L = BRR; // Lower Register (full byte)
    UBRR0 = BRR;
    
    UCSR0B = (1 << TXEN0); // transmitter enable for midi out
    UCSR0C = (1 << UCSZ01) | (1 << UCSZ00); // select 8-bit character size, start and stop bit defaults are correct, asynchronous

    while (1) //to infinity and beyond!
    {
        if(interruptsEnabled == 1)
        {
        // interrupt has occurred need to
        midiTransmit(clock_tick); // send clock tick data
        interruptsEnabled = 0;    //clear flag for next button push
        }
    }
}

ISR(INT1_vect)    //Interrupt Service Routine - used to toggle LED when button is pressed
{
    
    PORTB ^= (1 << PORTB5);    //toggle LED
    interruptsEnabled = 1;    //set flag so ISR is disabled once returned to main 
    
}

 

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Hold on a minute though. What you had previously meant that for the whole time you were doing midiTransmit() it was acting a debounce delay (because there couldn't be another interrupt while the transit was happening). Now you have no obvious form of debounce.

 

(which is why it is a dreadful idea to connect a button to INT1 - user timer polling with debounce for button inputs - "tact" switches are especially bad for bouncing!!)

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

meslomp wrote:

 

second you can just do

UBRR0 = BRR;

 

Can you elaborate on this point? Is this because UBRR0 has an 8bit size by default for the lower register?

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
onnect a button to INT1
clawson wrote:

Hold on a minute though. What you had previously meant that for the whole time you were doing midiTransmit() it was acting a debounce delay (because there couldn't be another interrupt while the transit was happening). Now you have no obvious form of debounce.

 

(which is why it is a dreadful idea to connect a button to INT1 - user timer polling with debounce for button inputs - "tact" switches are especially bad for bouncing!!)

 

This comment got us thinking quite a bit. Why is it a bad idea to connect a button to INT1 as opposed to polling?

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 1

Google the terms "Ganssle debounce" (and then search for the same thing on Freaks to read 500..1000 prior threads on the subject)

 

And UBRR0 is a "joined" 16 bit register pair made up of UBRR0L and UBRR0H. When the compiler is asked to write to it then it will actually do writes to the two separate register halves and, importantly, in the right order (some pairs are very strict and the order they are written). The AVR header files for avr-gcc generally define "pairs" for all grouped registers so if there is something like TCNT1L and TCNT1H then there will likely be a TCNT1 that, when written, writes a 16 bit value split across the two registers. So instead of:

TCNT1L = (12345 & 0xFF);
TCNT1H = (12345 >> 8);

you can use the much easier to read:

TCNT1 = 12345;

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

To elaborate further, many 16-bit registers (e.g. many timer registers) must be read in the order LO-HI, and written in the order HI-LO.  The compiler knows this and generates the proper code if you just write e.g. T1CNT = 12345, or val = T1CNT.

 

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

what happens if you do this as your main loop:

    while (1) //to infinity and beyond!
    {
        _delay_ms(500);
        midiTransmit(clock_tick); // send clock tick data        
    }

Start with this...... as if this does not function there is no need to add anything like responding to a button.

Then first add a simple if instruction that checks if the button is pushed( still nothing with interrupts)

 

    while (1) //to infinity and beyond!
    {
        if(<<is button pushed>>)
            
        {        midiTransmit(clock_tick); // send clock tick data        
                _delay_ms(500);
        }
    }

If that all works bring back the led toggling as that is a basic debugging tool you can use to see what is going on... put it in a separate routine for max flexibility and when it works do not touch it as you know that specific routine is doing its task.

 

After that you can expand further by replacing the if with a separate function that gives you a value of button pressed or not...

 

and so slowly expand again till the point were things break down again.

 

Should not take that long specially with the small program you have.

 

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

meslomp wrote:

what happens if you do this as your main loop:

    while (1) //to infinity and beyond!
    {
        _delay_ms(500);
        midiTransmit(clock_tick); // send clock tick data        
    }

Start with this...... as if this does not function there is no need to add anything like responding to a button.

Then first add a simple if instruction that checks if the button is pushed( still nothing with interrupts)

 

 

EDIT:

So when I tried this as my while loop it is not able to transmit midi either.

 

But we did find a solution! We tried changing the BAUDRATE constant to 9600 and were able to transmit the clock data but note data was nonsense. What fixed it was defining the midi variables using hex instead of binary and using 31250 as the baudrate. Thanks to everyone who had suggestions for our code. Useful stuff all around for a beginner like me!

 

 

 

 

Last Edited: Fri. Sep 18, 2020 - 07:34 PM