Midi-Receiver general programming issue... A few questions

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

Hello lovers of the code,

 

currently I have a midi receiver based on a Atmega8 which is behaving weird. It might be just a very tiny thing but I'm having a hard time understanding it.

This is the receive part from a much bigger programm, I cut it out so it's easier to understand.

 

config.h first. Just a few midi related things and a struct holding the config which gets read from eeprom.

#ifndef _CONFIG_H
#define _CONFIG_H
#include <avr/eeprom.h>

#define F_CPU           8000000
#define BAUD            31250L

#define MIDI_CHAN_INIT  0
#define PC_BYTE         0xc0
#define CC_BYTE         0xb0
#define PC_STATUS_INIT  (PC_BYTE | MIDI_CHAN_INIT)
#define CC_STATUS_INIT  (CC_BYTE | MIDI_CHAN_INIT)
#define RX_BUFFER_SIZE  32

typedef struct {
    volatile uint8_t midi_chan;
    volatile uint8_t pc_status;
    volatile uint8_t cc_status;
} conf_t;

conf_t ee_conf EEMEM = {
    .midi_chan = MIDI_CHAN_INIT,
    .pc_status = PC_STATUS_INIT,
    .cc_status = CC_STATUS_INIT
};

#endif /* _CONFIG_H */

 

main.c

#include <avr/io.h>
#include <stdio.h>
#include <stdint.h>
#include <stdlib.h>
#include <string.h>
#include "config.h"
#include <util/setbaud.h>
#include <avr/interrupt.h>
#include <util/delay.h>

conf_t conf;
uint8_t cmd_int;
uint8_t data1_int;
uint8_t data2_int;
uint8_t rx_message;
uint8_t rx_midi_complete = 0;
uint8_t rx_buffer[RX_BUFFER_SIZE];
uint8_t rx_write_index = 0;
uint8_t rx_read_index = 0;
uint8_t data_cnt = 0;
uint8_t foo = 0;

static inline void uartSend(uint8_t byte) {
   while (!(UCSRA & _BV(UDRE))) {};
   UDR = byte;
}

// send midi message
static void sendMsg(uint8_t index, uint8_t status, uint8_t data1, uint8_t data2) {
        uartSend(status);
        uartSend(data1);
        if(status == conf.cc_status){
            uartSend(data2);
        }
}

// send a string for debugging purpose
static void sendDebugString(const char *text) {
    while(*text){
        uartSend(*text++);
    }
}

static void init(void) {
    // read conf from eeprom
    eeprom_read_block(&conf, &ee_conf, sizeof(ee_conf));
    // build MIDI PC/CC status from PC/CC byte and MIDI channel byte
    conf.pc_status = (PC_BYTE | conf.midi_chan);
    conf.cc_status = (CC_BYTE | conf.midi_chan);
    //          transmit    receive     receive complete
    UCSRB = _BV(TXEN) | _BV(RXEN) | _BV(RXCIE);
    // frame format 8 data bit 1 stop bit
    UCSRC = (1 << URSEL) | (1 << UCSZ0) | (1 << UCSZ1);
    // set MIDI baud rate
    //UBRR0 = UBRR_VAL;                                     // atmega168 without setbaud.h
    //UBRRH = UBRR_VAL >> 8;                                // atmega8 without setbaud.h
    //UBRRL = UBRR_VAL & 0xFF;                              // atmega8 without setbaud.h
    UBRRH = UBRRH_VALUE;                                    // atmega8 with setbaud.h
    UBRRL = UBRRL_VALUE;                                    // atmega8 with setbaud.h
    // enable interrupt
    sei();
}

ISR( USART_RXC_vect ){
    // wait until message is received
    while ((UCSRA & (1 << RXC)) == 0) {}
    //read UDR to buffer & increment write index
    rx_buffer[rx_write_index++] = UDR;
    // if write index reached max buffer size set index to 0
    if (rx_write_index >= RX_BUFFER_SIZE){
        rx_write_index = 0;
    }
    // overflow situation
    if (rx_write_index == rx_read_index){
        sendDebugString("overflow");
    }
}

static void readMidi(void){
    // if data is available
    if (rx_read_index != rx_write_index){
        // for debugging send i to check if condition was met
        sendDebugString("i");
        // counter to see how often readMidi gets iterated
        uartSend(foo);
        // disable interrupt for increase of read buffer
        cli();
        // read message from buffer
        rx_message = rx_buffer[rx_read_index++];
        // send message for debugging
        //uartSend(rx_message);
        // if read index reached max buffer size set index to 0
        if (rx_read_index >= RX_BUFFER_SIZE){
            rx_read_index = 0;
        }
        // enable interrupt again
        sei();
    }

    // build the MIDI message
    // 1 - determine MIDI status (command)
    if((rx_message == PC_STATUS_INIT || rx_message == CC_STATUS_INIT) && (!cmd_int && !rx_midi_complete) ){
        // send for debugging
        sendDebugString("1");
        // counter to see how often readMidi gets iterated
        uartSend(foo);
        // increase foo for next readMidi iteration
        foo++;
        // set status (command) byte
        cmd_int = rx_message;
        // send for debugging
        uartSend(rx_message);
    // 2 - determine data1 MIDI PC
    }else if((cmd_int == PC_STATUS_INIT) && (!rx_midi_complete)){
        // send for debugging
        sendDebugString("2");
        // counter to see how often readMidi gets iterated
        uartSend(foo);
        // increase foo for next readMidi iteration
        foo++;
        // set data1
        data1_int = rx_message;
        // send for debugging
        uartSend(rx_message);
        // PC message only has a data1 byte - MIDI message is complete
        rx_midi_complete = 1;
    // 3 determine data1 for MIDI CC
    }else if((cmd_int == CC_STATUS_INIT) && (!data_cnt) && (!rx_midi_complete)){
        // send for debugging
        sendDebugString("3");
        // counter to see how often readMidi gets iterated
        uartSend(foo);
        // increase foo for next readMidi iteration
        foo++;
        // set data1
        data1_int = rx_message;
        // send for debugging
        uartSend(rx_message);
        // counter to check if data1 was already set in the above if statement
        data_cnt++;
    // 4 determine data2 for MIDI CC
    }else if((cmd_int == CC_STATUS_INIT) && (data_cnt) && (!rx_midi_complete)){
        // send for debugging
        sendDebugString("4");
        // counter to see how often readMidi gets iterated
        uartSend(foo);
        // increase foo for next readMidi iteration
        foo++;
        // set data2
        data2_int = rx_message;
        // send for debugging
        uartSend(rx_message);
        // CC message has a data1 and a data2 byte - MIDI message is complete
        rx_midi_complete = 1;

    }
    // send MIDI message if it is complete and reset all variables
    if(rx_midi_complete){
        //sendMsg(0, cmd_int, data1_int, data2_int);
        rx_message = 0;
        data_cnt = 0;
        cmd_int = 0;
        data1_int = 0;
        data2_int = 0;
        rx_midi_complete = 0;
    }
}

int main(){
    init();
    while(1){
        readMidi();
    }
    return 0;
}

 

I'm using MIDI Ox to send messages over a USB-MIDI-interface and a cheap logic analyser to see the result because MIDI Ox displays only correct midi messages

 

Input - three sequential MIDI messages

'176' //CC message
'0'	// data1
'0'	// data2
'176' // CC message
''	// data1
'0'	// data2
'192' //PC
'0'	// data1

 

Output - with all the debugging output it seems to be ok

 

i	// indices not equal - read from buffer
'0'	// foo 0 first iteration
'176'	// read 176 from buffer
1	// 176 is CC "determine MIDI status" matched
'0'	// foo still 0
'176'	// set cmd_int to 176
i	// indices not equal - read from buffer
'1'	// foo 1 second iteration
'0'	// read 0 from buffer
3	// "determine data1 for MIDI CC" matched
'1'	// foo still 1
'0'	// set data1_int to 0
i
'2'
'0'
....

When I remove the debugging output except the digit to determine the if statement and the message it seems that in the beginning readMidi() gets iterated twice without reading the buffer twice

1	// "determine MIDI status" matched
'176'	// set cmd_int to 176
3	// "determine data1 for MIDI CC" matched
'176'	// set data1_cmd to 176
4
'0'
1
'176'
3
' '
i
4
'0'
1
'192'
2
'0'

When I send the Midi messages a few more times afterwards it's always the first 176 that is displayed twice.

I thought that maybe readMidi() is faster than the interrupt so I tried to put the complete assembly of the Midi message into the code block of this if statement

 

if (rx_read_index != rx_write_index){

Right after sei(); but then I don't get an output at all and I don't know why...

readMidi() shouldn't be faster than the ISr because it's always the first 176 that's in there twice.

 

When I remove all the debugging output and enable sendMsg() then I get this

'176'
'176'
'176'
'176'
'176'
'176'
'192'
'192'
'192'

I strongly thing that uart gets messed up the way I use it.

Can anyone give me a hint on what's wrong here?

This topic has a solution.
Last Edited: Thu. May 14, 2020 - 07:06 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You do some strange things. Here's one:

  while ((UCSRA & (1 << RXC)) == 0) {}

The interrupt has already told you there is a byte available.

 

You don't atomically read from the circular buffer

You probably want to use a finite state machine to decode the midi messages.

 

My suggestion is to look at the midi library for Arduino to see how it does things.

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

Kartman wrote:

You do some strange things. Here's one:

  while ((UCSRA & (1 << RXC)) == 0) {}

The interrupt has already told you there is a byte available.

That line was a question I wanted to ask later if it is needed when using the interrupt. Thanks for clarification.

Kartman wrote:

You don't atomically read from the circular buffer

You probably want to use a finite state machine to decode the midi messages.

 

My suggestion is to look at the midi library for Arduino to see how it does things.

I modified readMidi() a bit:

static void readMidi(void){

    if (rx_read_index != rx_write_index){
        ATOMIC_BLOCK(ATOMIC_FORCEON){
            rx_message = rx_buffer[rx_read_index++];
            if (rx_read_index >= RX_BUFFER_SIZE){
                rx_read_index = 0;
            }
        }

        switch(rx_message_byte){

            case CMD:
                if(rx_message == PC_STATUS_INIT || rx_message == CC_STATUS_INIT){
                    cmd_int = rx_message;
                    rx_message_byte = DATA1;
                }
            case DATA1:
                if(cmd_int == PC_STATUS_INIT){
                    data1_int = rx_message;
                    uartSend(rx_message);
                    rx_message_byte_next = END;
                }else if(cmd_int == CC_STATUS_INIT){
                    data1_int = rx_message;
                    uartSend(rx_message);
                    rx_message_byte = DATA2;
                }
                break;
            case DATA2:
                if(cmd_int == CC_STATUS_INIT){
                    data2_int = rx_message;
                    uartSend(rx_message);
                    rx_message_byte = END;
                }
                break;
            case END:
                sendMsg(0, cmd_int, data1_int, data2_int);
                rx_message = 0;
                cmd_int = 0;
                data1_int = 0;
                data2_int = 0;
                rx_message_byte = CMD;
                break;
        }
    }
}

In this example I put the assembly of the Midi message in the block of

if (rx_write_index == rx_read_index){

I removed cli() and sei() and used ATOMIC_BLOCK. still no output. When I the finite state machine (in case I did it correct) after this block I get the same result as before, an infinite loop.

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

An i finite loop Where?

Your code assumes the midi bytes will arrive in the order you expect. In a perfect world they would. In the real world it is not guaranteed.

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

The loop.. yes I know..

 

    while(1){
        readMidi();
    }

switch case gets always called and prints out 0 because rx_message is always 0 when no message is comming in.

That's why I wanted to put it in the if block where rx_read_index != rx_write_index is asked. To only put out data, when data is in the buffer. But it does not work.

The data arrives always in the same order that I expect it, at least with the Midi simulator. How else could I check if the data from the buffer is data1 of a CC or PC ?

There is no flag or whatsoever in midi data.

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

Your rx_read_index is only updated in non interrupt code, and it's a uint8_t so any update is atomic, therefore you don't actually need to block interrupts when updating rx_read_index.

But I don't think that's your problem, the problem is in the rest of the code somewhere.

 

The switch looks a bit suspicious.

There is no break in case CMD so it falls through to case DATA1, is it meant to fall through there?

It only gets to END after receiving the next byte. Is that intended?

 

In general, I would go for a pattern like

static void readMidi(void)
{
    if (data_is_available())
    {
        uint8_t c = read_next_char();

        /* then do something with c */
    }
}

where 'do something with c' would mean run through the state machine

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

I will try what you suggested but once in a while a while loop is needed. It works like this (doesn't matter if in a ATOMIC_BLOCK or not)

static void readMidi(void){
    while(rx_read_index != rx_write_index){
        ATOMIC_BLOCK(ATOMIC_FORCEON){
            rx_message = rx_buffer[rx_read_index++];
            if (rx_read_index >= RX_BUFFER_SIZE){
                rx_read_index = 0;

            }
        }
        switch(rx_message_byte){
            case CMD:
                if(rx_message == PC_STATUS_INIT || rx_message == CC_STATUS_INIT){
                    cmd_int = rx_message;
                    rx_message_byte = DATA1;
                }
                break;
            case DATA1:
                if(cmd_int == PC_STATUS_INIT){
                    data1_int = rx_message;
                    midi_msg_complete = 1;
                }else if(cmd_int == CC_STATUS_INIT){
                    data1_int = rx_message;
                    rx_message_byte = DATA2;
                }
                break;
            case DATA2:
                if(cmd_int == CC_STATUS_INIT){
                    data2_int = rx_message;
                    midi_msg_complete = 1;
                }
        }
        if(midi_msg_complete){
            sendMsg(0, cmd_int, data1_int, data2_int);
            midi_msg_complete = 0;
            rx_message = 0;
            cmd_int = 0;
            data1_int = 0;
            data2_int = 0;
            rx_message_byte = CMD;
        }
    }
}

 

You are right a break; was missing and END was never called because that would imply that something would have been read from buffer.

I', still wondering, why

if(rx_read_index != rx_write_index){

is not working at all. Not once. I placed a uartSend(); right after the ATOMIC_BLOCK block but it does not even send this one message.

Instead if there are more messages beeing written into the buffer there is an overflow.

 

 

Last Edited: Wed. May 13, 2020 - 03:15 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

tuckito wrote:
You are right a break; was missing

GCC can warn you about that:

-Wimplicit-fallthrough=n

Warn when a switch case falls through. 

(and provides a way to flag when you're doing it deliberately)

 

https://gcc.gnu.org/onlinedocs/gcc/Warning-Options.html  

 

EDIT

 

-Wimplicit-fallthrough=3  is enabled by   -Wextra

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...
Last Edited: Wed. May 13, 2020 - 03:22 PM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

tuckito wrote:

I', still wondering, why

if(rx_read_index != rx_write_index){

is not working at all. Not once. I placed a uartSend(); right after the ATOMIC_BLOCK block but it does not even send this one message.

Ah, might be because rx_write_index needs to be volatile.

If it isn't volatuile, it depends how smart the optimisation is whether you get away with it or not.

 

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

Yes volatile helps to use the if statement!

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

@Kartman @MrKendo @awneil: thank you guys a lot it would have cost me hours if not days to get it running.

Even if I used all of your suggestions the solution to the first code I posted is the volatile keyword to get code running properly when I put the decoding of the midi message right into the index comparison block.