Help with arrays in EEPROM...

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

Hi All!

 

I've been stuck on a problem for a week now so it's time to ask for help.

 

I want to store 128 user-definable presets in EEPROM. Each preset is 16 bits and the exact preset being stored or retrieved will be called by a number (0-127) recieved by USART. I figure the easiest way to do this is by declaring a preset array of 128 values, then I can store and recall simply by using the variable we read the UDR into as the array number. Here's some code for illustration:


uint16_t EEMEM preset_eeprom[128];  //eeprom preset array

uint8_t preset_number;              //preset number
uint16_t preset_data;               //preset data

int main(void)
{
    //actually done in an ISR
    preset_number = UDR0;   //Read recieved data and store in preset_number
    
    //recall data for processing
    preset_data = eeprom_read_word(&preset_eeprom[preset_number]);
    //do stuff with it
    
    //store data
    eeprom_update_word(&preset_eeprom[preset_number], preset_data);
    
}

It doesn't work, however, I can read and write to EEPROM with other variables and even a structure no problem. I know I am getting the data from the USART correctly because I wrote a function to blink and LED the "preset_number" of times. So it's gotta be the way I'm trying to use the array. Do we use & here or (uint16_t*) or (void*)? This question at stack exchange has me confused even though it seems to confirm I'm on the right track; see "//macro for easier usage" in the code: http://electronics.stackexchange.com/questions/100887/how-to-store-array-of-values-in-eeprom-of-atmega16-and-then-read-it-in-sequence

 

Any ideas what I'm doing wrong? Also, is there anyway to read the current contents of the EEPROM in Atmel Studio6? This would help me see if the problem is occurring saving the preset or recalling it, or both.

 

Thank you in advance!

This topic has a solution.
Last Edited: Tue. Aug 11, 2015 - 02:58 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
    preset_data = read_eeprom_word(&preset_eeprom[preset_number]);
    update_eeprom_word(&preset_eeprom[preset_number], preset_data);

Huh?

 

Don't you mean?:

    preset_data = eeprom_read_word(&preset_eeprom[preset_number]);
    eeprom_update_word(&preset_eeprom[preset_number], preset_data);

 

 

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

joeymorin wrote:

    preset_data = read_eeprom_word(&preset_eeprom[preset_number]);
    update_eeprom_word(&preset_eeprom[preset_number], preset_data);

Huh?

 

Don't you mean?:

    preset_data = eeprom_read_word(&preset_eeprom[preset_number]);
    eeprom_update_word(&preset_eeprom[preset_number], preset_data);

 

 

 

Haha, ya. Just typed it wrong for the example. I'll edit the OP.

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

Sothis......

making things up is making us guess........

 

post the part of the real code you are having trouble with, it might be that you just made a typo and all else is correct.

please never just make up an illustration when you ask a question about why your code is not working, with illustrations we cannot do a thing.

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

Quote:
Just typed it
Never type.  Always copy/paste.  Nobody likes to chase down ghosts.  Post the actual code you are having trouble with.  Don't retype it.  Copy/paste it.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Well when you have 6000 lines of code, it's not practical to post all that, especially when 10 lines are relevant.

 

So, let's try a new approach. How do you access a single word from an array stored in EEPROM when the array uses EEMEM? Obviously, eeprom_read_word (This_part_is_my_question) and eeprom_update_word (This_part_is_my_question, little foggy here too). That's it. I'm stuck on when to use &something, (void*)something, (uint8_t*) something.

 

"&something" works for the other variables retrieved from EEPROM, but for the structure, I had to use "(const void*)something" before I was using EEMEM, and after "(const void*)&something rather than "&something". The code in the stack exchange link above gives just &something[array_number], but it also shows a macro that confuses me:

 

#define read_eeprom_word(address) eeprom_read_word ((const uint16_t*)address)

 

 

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

Well when you have 6000 lines of code, it's not practical to post all that, especially when 10 lines are relevant.

Agreed but in this case what you do its write yourself a 10 line test program to isolate the issue and allow you to present it to others. Often in doing so you (a) realise that it's not just the 10 lines but some interaction with the other 5,990 where the real problem lies or (b) solve the issue in your generation of the test code as it suddenly becomes very obvious what you are doing wrong when you sort the wheat from the chaff.

 

Here comes such a 10 liner:

#include <avr/eeprom.h>

int eedata[5] EEMEM = { 12345, 23232, 21359, 14235, 11111 };

int main(void) {
    int element;
    element = eeprom_read_word(&eedata[2]); // element = 21359
    element = 16441;
    eeprom_update_word(&eedata[0], element); // 12345 replaced by 16441
}

(OK, less than 10 lines then! EDIT: actually with the vertical white space I used I suppose it IS exactly 10 lines?)

 

I picked 12345 and 16441 to demonstrate something else too. 12345 is 0x3039 and 16441 is 0x4039 so when I use eeprom_update_word() to write the 16441 value the 0x30 will be replaced by 0x40 but the 0x39 will remain untouched - that is the difference between using eeprom_update_word() and eeprom_write_word() and is the reason why "update" would almost always be preferred.

 

As for using a #define to rename eeprom_read_word() (that all avr-gcc programmers recognise, know and love) to read_eeprom_word seems like a maintenance nightmare in the making. What possible merit is there in having two names for the same thing - one well known, one completely obscure?

Last Edited: Tue. Aug 4, 2015 - 08:56 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Haha, thank you Clawson! I honestly never thought about making a test program before...I just kinda plan out what I want to do and keep at it until it works.

 

Agreed that #define is not a good thing. What is confusing about it to me is not so much the changing of the name (makes sense you can't use the same call), but if I follow it right, then the actual correct usage of update would be

 

eeprom_update_word((uint_16t*)&something[array_num], something_to_store)

 

and not

 

eeprom_update_word (&something[array_num], something_to_store).

 

Is this correct? I was doing the latter but the former didn't work either. 

 

Is there a way to read the eeprom contents of a chip to see if in fact array data is being stored? 

 

 

Last Edited: Tue. Aug 4, 2015 - 05:39 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
Is there a way to read the eeprom contents of a chip to see if in fact array data is being stored?

avrdude <all-your-other-avrdude-options> -U eeprom:r:foo.bin:r

hexdump -C foo.bin

 

However, your next step it to follow the advice you've been given:  Write a small test program to expose the problem.  If you cannot succeed in replicating the problem that way, it would seem clear that the problem doesn't lie with EEPROM/EEMEM/etc., but with the rest of your program.

 

You've been a member for almost 2 years.  Have you not yet read and understood this?  And this?

 

Quote:
I just kinda plan out what I want to do and keep at it until it works.

“ Without requirements or design, programming is the art of adding bugs to an empty text file.

 

EDIT: sp

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Tue. Aug 4, 2015 - 08:48 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@joeymorin: With all due respect, you've been a bit insulting rather than helpful right out of the gate. Let's recap what I've done in an effort to solve the problem before bringing it here to the forum:

 

1. Downloaded all of Dean Camera's wonderful tutorials and read them multiple times before programming anything.

2. Because reading/writing a specific member of an array in EEPROM is not covered in the TUT, I've googled as much as I can think of looking for the solution.

3. Gone through my code both on paper and in the IDE trying new things I've learned from research.

4. Included error checking code to debug the software and isolated the problem down to the way I'm trying to read and write to an array in EEPROM.

    For instance:

     -  I know the ISR receives the right array_number because I set up code to blink an LED the array_number times and it is always correct

     -  I know the right functions are called by placing that same blink code in the start of the code to show that function was in fact called.
     -  I also know that nothing is being stored in EEPROM at the desired eeprom_array[preset_num] location because I set up code that says "if eeprom_array[preset_num] equals #FFFF" blink the LED. #FFFF would indicate that the EEPROM at that location has not been written to as that is the default.

     -  I know that it isn't other parts of the code because the after we get a preset_num command via USART, we fetch the data from EEPROM and feed it to the same function that initializes the unit (which works fine).

 

Further:
1. My original post gives background on what I am trying to do, why I think it is the call to read and write a single member of an array stored in eeprom and not anything else, and I ask three questions (which can be found by looking for the '?' at the end of the sentence).

2. Those questions don't say: "will someone debug my code for me?", the one asks "I don't think eeprom_update_word(&something, something) is right, can someone explain when to use &something versus ((unint16_t*)&something, vs. something else?".

3. Of the 5 responses to my questions, only clawson attempted to answer them but clawson is a rockstar like that. The rest have been "you didn't ask your question correctly".

4. The OP clearly asks "Also, is there anyway to read the current contents of the EEPROM in Atmel Studio6?", not avrdude.

I'm asking questions rather than saying "debug my code please" because "debug my code" examples tend to be application specific, and not easy for someone learning to look at the example and apply it to their problem. The one google hit on reading/writing/updating arrays has the macro which will likely confuse someone starting out. My goal in asking to learn the syntax of this operation will hopefully lead to the post helping others who also google: "AVR EEPROM arrays".

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

...because I set up code that says "if eeprom_array[preset_num] equals #FFFF" blink the LED. #FFFF would indicate that the EEPROM at that location has not been written to as that is the default.

 Show this code, ideally distilled down to a complete test program, that writes value(s) to the array and then this check code--are your really addressing eeprom_array[] "directly", and not by fetching the value?

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

As far as I know, the AS6 programming dialog can read the EEPROM to a hex file and only via a genuine Atmel programmer. You would need to post-process this if you want to see or edit the contents like normal human being.

The Codevision IDE has its own programming dialog that copes with both Atmel and third party programming hardware. Having read the flash or eeprom, you can view the buffer.

An alternative is to start a debug session in AS6. Then view the appropriate memory window. The last time that I tried this, AS6 did not load the .EEP file. AS4 did load eeprom ok. Try it for yourself with the current AS6 or AS7.

I am wary of doing this for you. I do not want to get my head bitten off.

David.

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

If you can't provide a complete problem description including a complete test program which demonstrates that problem, you're going to have a harder time finding people to help you for free.  The code fragment you originally posted (when edited to correctly use real functions provided by <avr/eeprom.h>, and with other 'details' like #include <avr/io.h>) builds working code which does not demonstrate your problem.  Are we supposed to guess?

 

Suit yourself.  Good luck with your project.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

4. The OP clearly asks "Also, is there anyway to read the current contents of the EEPROM in Atmel Studio6?", not avrdude.

 Is that what you are looking for?

 

Now, I don't normally use Studio/GCC toolchain.  In my usual CodeVisionAVR toolchain, I have "transparent" EEPROM facilities so I dodge your problem area for the most part, and also the ISP part of the IDE has editors for flash/EEPROM. 

 

Poking at the facility above, it appears to want to make a .EEP file, that appears to be in Intel HEX format.  You'll have to decode on your own (shouldn't be a problem for checking a few bytes if the address is known), or use another transformation tool as earlier mentioned.

 

:1000000000000000020000070201000702020007D2
:1000100002030007020400070205000702060007AA
:10002000020700070208000702090007020A00078A
:10003000020B0007020C0007020D0007020E00076A
:10004000020F000700000000000000000000000098
...

 

Re your addressing questions (when to use pointer cast or not):  Other than a warning, perhaps, it shouldn't be a problem as long as your expression resolves to the correct address.

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

Last Edited: Tue. Aug 4, 2015 - 08:51 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It might be quite interesting to look at the .MAP file from your test program build, to see where your array ends up.

 

Another sanity check would be to use a few hard-coded array indices in the test program for update/read operations, and see if those results are better.  (The compiler will probably resolve to a hard address.)  Then, jam the same values into the variable used for array indexing ("preset_number"?).  Making it volatile will force the compiler to do the indexing at run time.  Does that "work"?  Now you'd have a manegable-sized test program, and the .LSS could be examined for clues.

 

I know you said you verified that preset_number was set correctly but as a guess it ain't what you think it should be.  (>>if<< your posted fragment in the OP actually represents the real app code)

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

@joeymorin: With all due respect, you've been a bit insulting rather than helpful right out of the gate. Let's recap what I've done in an effort to solve the problem before bringing it here to the forum:

Ironic. Joey is possibly the most patient, helpful person here. Guess I'm out too.

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

@theusch, @David: Thank you for answering the questions!

 

theusch wrote:
Show this code, ideally distilled down to a complete test program, that writes value(s) to the array and then this check code--are your really addressing eeprom_array[] "directly", and not by fetching the value?

I will work on this tonight for you. To answer, no I fetch the value first, split it, then check. I'll show that in the code for you too.

 

david.prentice wrote:

I am wary of doing this for you. I do not want to get my head bitten off.

 

Haha, no worries. I'm not trying to bite any heads. My intention is to not to burden any members of the forum, I don't mind doing the hard work because being stuck on something and figuring it out is learning, better than the answer being given without any hardship. So I did the hard work, tried to condense the question to "Is this the right syntax for the array EEPROM reads/write/updates (the code example), and why do the stack exchange macros add the typecast (i think that is what it is)? If the code in the OP is the correct syntax and we don't need to worry about those lines being the problem, then I can look elsewhere. That's all I wanted to know. The surrounding code makes no difference if you aren't using read/write/update EEPROM correctly.

 

theusch wrote:

 Is that what you are looking for?

Yup, I tried that yesterday but AS6 didn't claim the .EEP file; windows asked to find the program...I went on brief google expedition and gave up.

 

As for the rest of your advice:

 

I don't know how to read the .map file but I can learn. I'll try this.

 

Hardcoding some parts of the array is brilliant. Could you expand on what should be made volatile? If my read/write/update syntax is correct, then I wonder if one of the variables I'm using during the data parsing/handoffs isn't volatile when it should be.

 

That's my same hunch too...maybe I'm storing correctly to array[5] for example, but reading another array...which would still be default 0XFFFF. I looked for this a bit, but it could be hiding as a variable not declared as volatile, which i hadn't considered before the above post.

Thank you again, theusch. Got a lot of new hope for a solution thanks to your posts.

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

Yup, I tried that yesterday but AS6 didn't claim the .EEP file

??? "Claim"?  I have no idea what you are talking about.

 

As I said, it is Intel HEX, so any text editor would work.  I posted the fragment from Wordpad.

 

You are thanking me for my help, but it all boils down to posting a complete test program that demonstrates your

suspect behaviour. 

I don't know how to read the .map file but I can learn. I'll try this.

Actually, since you seem to be quite particular in questions and answers, I just asked to "see" it.  The gurus can decipher.   (But how hard would it be to find the name of your EEPROM array with a text-editor search?)

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

joeymorin wrote:

If you can't provide a complete problem description including a complete test program which demonstrates that problem, you're going to have a harder time finding people to help you for free.  The code fragment you originally posted (when edited to correctly use real functions provided by <avr/eeprom.h>, and with other 'details' like #include <avr/io.h>) builds working code which does not demonstrate your problem.  Are we supposed to guess?

 

Suit yourself.  Good luck with your project.

 

I'm sorry but it is extremely overwhelming to write a test program from the software/hardware I have. I don't know what to provide you with, what not to provide you with, how to test the test program in the device since there's all of the initializations specific to the hardware that would be clutter for you as well. Looks like my attempt to make the problem as easy as possible for everyone actually made it harder. I'll get some test code (cut and pasted from the actual program) and post back.

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

theusch wrote:

??? "Claim"?  I have no idea what you are talking about.

Meaning I found the folder it saved to, double clicked it, and windows said "can't find program to open this file with". I thought AS6 would recognize the extension as it's own file type.

 

theusch wrote:

Actually, since you seem to be quite particular in questions and answers, I just asked to "see" it.  The gurus can decipher.   (But how hard would it be to find the name of your EEPROM array with a text-editor search?)

Ahhhh, I thought you were telling me to look at it. I'll try to find it and post it for you.

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

Ok, here's the test code condensed as much as I can condense it.

#define F_CPU 8000000UL

#include <avr/io.h>
#include <avr/interrupt.h>
#include <avr/eeprom.h>

#define CLEAR_MIDI_STATUS_BYTE 0x00
#define CLEAR_MIDI_DATA_BYTE 0x80

//	Global Variables
uint16_t EEMEM midi_preset_eeprom[128];			//Used to store presets in memory
volatile uint8_t midi_status_byte = 0;			//Gets MIDI Status Byte
volatile uint8_t midi_data_byte_0 = 0;			//Gets first MIDI Data Byte
volatile uint8_t program_change_received = 0;	//Used to tell software a program change was received

// Called by main to get the stored data from EEPROM and load
void validate_Midi_Receive(uint8_t midi_data_byte_0)
{
	//Local Variables
	uint16_t midi_preset_read = 0;				//Used to store the midi preset data from EEPROM in memory
	uint8_t midi_preset_activeH = 0;			//Used to store the activeH portion of midi_preset_read
	uint8_t midi_preset_activeL = 0;			//Used to store the activeL portion of midi_preset_read

	midi_preset_read = eeprom_read_word(&midi_preset_eeprom[midi_data_byte_0]); //Gets the preset data from eeprom for the received preset number (midi_data_byte_0)
	
	//Parse word into 2 bytes
	midi_preset_activeH = ((midi_preset_read >> 8) & 0xFF);		//Shifts activeH portion of m_p_r into m_p_a
	midi_preset_activeL = (midi_preset_read & 0xFF);			//Shifts activeL portion of m_p_r into m_p_a
	
	
	if((midi_preset_activeH != 0xFF) && (midi_preset_activeL != 0xFF))		//Checks to see if there is MIDI Data stored here, default eeprom is 0xFF, 0xFF is not valid config
	{
		/*Success is in here*/
	}
}

//Called by main to wait for the location to store
uint8_t validate_Midi_Store (void)
{
	//Local Variables
	uint8_t activeH = 0;			//Temp variable to shift data into midi_preset
	uint8_t activeL = 0;			//Temp variable to shift data into midi_preset
	uint16_t midi_preset = 0;		//Used to read and write the current midi_preset
	uint8_t midi_preset_number = 0; //Used to store midi preset in correct location in memory

	for (;;)
	{
		// Listen for preset number to come in that we are storing to
		if (program_change_received == 1)		//Check to see if the received byte flag is set
		{
			while(midi_data_byte_0 == CLEAR_MIDI_DATA_BYTE){}		//wait to get the program change data
	
			midi_preset_number = midi_data_byte_0;					//store program change data as midi preset number
	
			//Long code here to get activeH and activeL
 
			midi_preset = ((activeH & 0xFF) << 8) | (activeL & 0xFF); //shifts active into MSB and active_model into LSB

			program_change_received = 0;							//clear program_change_received flag
			midi_status_byte = CLEAR_MIDI_STATUS_BYTE;				//clear midi_data_byte
			midi_data_byte_0 = CLEAR_MIDI_DATA_BYTE;				//clear midi_data_byte_0
		}

		//store preset and exit on button press
		if (store_button_pressed)
		{
			eeprom_update_word(&midi_preset_eeprom[midi_preset_number], midi_preset);
			return 1;
		}
	}	 
}

int main(void)
{
	//Initialize Unit: Set ports, setup USART, setup interrupts, etc.
	
	//Local Variables
	uint8_t midi_store = 0;			//Used to both exit loop in validate_Midi_Store and debounce store button

	for(;;)
	{
		//Listen for buttons and other stuff

		//Listen for button to store
		if (store_button_pressed)
		{
			midi_store = validate_Midi_Store();
		}

		//Process the program change
		if (program_change_received == 1)		//Check to see if we have received a program change message
		{
			while (midi_data_byte_0 == CLEAR_MIDI_DATA_BYTE)	//wait for MIDI data byte
			
			validate_Midi_Receive(midi_data_byte_0);	//Send midi data to validate_Midi_Receive for processing
			
			//clear PCR, status byte and data byte
			program_change_received = 0;
			midi_status_byte = CLEAR_MIDI_STATUS_BYTE;
			midi_data_byte_0 = CLEAR_MIDI_DATA_BYTE;
			
		}
	}
}

// USART Receive Interrupt
ISR(USART0_RX_vect)
{
	char received_byte;				//Temp variable to process UDR data
	uint8_t last_byte_was_data = 0; //Used to only get the first data byte
	
	received_byte = UDR0;		//Store received data into recieved_byte for further processing

	//Test if status or data byte
	if (status_byte) 
	{
		//Test if program change message, and if we need to react to it
		if(program_change and midiChannel_matches or omniOn) 
		{
			program_change_received = 1;			//set program change received variable to 1
			midi_status_byte = received_byte;		//otherwise, midi_status_byte0 is ready for data
			last_midi_byte_was_data = 0;			//Last byte was a status byte
		}
		//otherwise ignore data until program change received we need to listen to
	}
	else //it's a data byte
	{
		//if we have received a program change in last status_byte
		if (program_change_received == 1)
		{
			//check to see if byte received was a data byte
			if (last_midi_byte_was_data == 0)			//If last midi byte was a status byte we need to store data
			{
				midi_data_byte_0 = received_byte;		//store data in midi_data_byte_0
			}
			else
			{
				last_midi_byte_was_data = 1;			//last MIDI byte was data, ignore until new status byte
			}
		}
		//otherwise ignore data until program change received that we need to listen to
	}
}

Here's the method of operation in case it helps follow the code:

1. Set the unit the way we want to store and hit the "store" button. Calls validate_Midi_Store.

2. The unit enters a loop that waits for the user to send the preset number to store to.

3. The store button is hit again and the unit writes the preset data to EEPROM location sent by user in 2. Back to main loop.

4. Assuming the settings have changed, the user sends the same preset number in 2. Calls validate_Midi_Restore. The unit fetches the data store in EEPROM at that number and changes the settings stored previously.

 

Combed over it before posting, still don't notice anything fishy (to me that is). Going to check .MAP file now.

 

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

When I try to build your above code, I get a number of errors:

$ avr-gcc -g -W -Wall -O1 -mmcu=atmega1284 -save-temps sothis.c -o sothis.elf
sothis.c: In function 'validate_Midi_Store':
sothis.c:65:13: error: 'store_button_pressed' undeclared (first use in this function)
         if (store_button_pressed)
             ^
sothis.c:65:13: note: each undeclared identifier is reported only once for each function it appears in
sothis.c: In function 'main':
sothis.c:85:13: error: 'store_button_pressed' undeclared (first use in this function)
         if (store_button_pressed)
             ^
sothis.c:78:13: warning: variable 'midi_store' set but not used [-Wunused-but-set-variable]
     uint8_t midi_store = 0;   //Used to both exit loop in validate_Midi_Store and debounce store button
             ^
sothis.c: In function '__vector_20':
sothis.c:115:9: error: 'status_byte' undeclared (first use in this function)
     if (status_byte)
         ^
sothis.c:118:12: error: 'program_change' undeclared (first use in this function)
         if(program_change and midiChannel_matches or omniOn)
            ^
sothis.c:118:27: error: expected ')' before 'and'
         if(program_change and midiChannel_matches or omniOn)
                           ^
sothis.c:122:13: error: 'last_midi_byte_was_data' undeclared (first use in this function)
             last_midi_byte_was_data = 0;   //Last byte was a status byte
             ^
sothis.c:110:13: warning: unused variable 'last_byte_was_data' [-Wunused-variable]
     uint8_t last_byte_was_data = 0; //Used to only get the first data byte
             ^

 

So obviously this is not a 'complete test program'.  A minimum requirement for such a program is that it compiles.  It seems unlikely that you've reproduced the stated problem behaviour with your posted code fragment.

 

Please provide (at least) declarations/definitions for:

store_button_pressed
status_byte
program_change
last_midi_byte_was_data
midiChannel_matches
omniOn 

 

 

Also, this

         if(program_change and midiChannel_matches or omniOn)

... is not C.

 

You might also like to mention which AVR you are building for.  Your use of USART0_RX_vect narrows it down a little, but there are still over 40 devices which fit that description.

 

But this is a good start!

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Wed. Aug 5, 2015 - 03:29 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@joeymorin: Thank you. See this is part of what I was afraid of. It compiles for me, but some if not all of those errors you got were me shorthanding trying to save you or anyone else the trouble of the long version.

 

For instance:

 if(program_change and midiChannel_matches or omniOn)

really is 

if ((received_byte & PROGRAM_CHANGE) == PROGRAM_CHANGE)
	{
		if (((received_byte & MIDI_CHANNEL_MASK) == midi_channel) || (omni_on == 1))
		{
			program_change_received = 1;			//set program change received variable to 1
				
			midi_status_byte = received_byte;		//otherwise, midi_status_byte0 is ready for data
				
			last_midi_byte_was_data = 0;			//Last byte was a status byte
		}
			
	}

 where PROGRAM_CHANGE and MIDI_CHANNEL_MASK are macros and midi_channel and omni_on are global variables...but I was already at 155 lines and thought it was too much. All of the rest of those errors are the exact same thing.

 

Is there a good way to shorthand or indicate button presses that are known to be working?

 

Also, is it better to edit the code above with fixes or post again below?

Last Edited: Wed. Aug 5, 2015 - 05:43 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

joeymorin wrote:

You might also like to mention which AVR you are building for.  Your use of USART0_RX_vect narrows it down a little, but there are still over 40 devices which fit that description.

 

It is the ATmega164A and I'm using the JTAGICE3 as an ISP.

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

sothis,

 

start a new project.

set-up the uart on it as your debug interface.

write a simple test program that dumps the eeprom content preferably hex -> ascii. (just read byte by bite from location 0 to end of eeprom)

tip: start a line with the first address then spit out 8 or 16 bytes and do a new line starting with the address.

 

then expand your program by writing a single byte, word and array to the eeprom.

each time dumping the data back to the PC on the uart.

 

this should give you better knowledge on how this all works.

 

all the above should take you far less than a day.

It took me 4 evenings as a very novice programmer to get this all up for eeprom, flash and ram  and it gave me some interesting insights on how this worked.

 

I do not have the code at work(just checked) and will not get home tonight until it is time to go to sleep I could only send you the program tomorrow evening and by then you should have been able to do the above and have found out how you can best do what you want.

Note that I also only used the tutorial written by dean so it will contain all the information you need to get going.

 

regards

 

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

It feels as though the spirit of the term 'test program' has not been communicated correctly.

 

Here is an example of one which attempts to expose the behaviour your have reported:

 

#include <avr/io.h>
#include <avr/eeprom.h>


#define PRESET_SZ 128



#define BAUD 9600
#include <util/setbaud.h>



uint16_t EEMEM preset_ee[PRESET_SZ];



static void uart_init(void) {
  UCSR0A = (USE_2X<<U2X0);  
  UCSR0B = (1<<RXEN0) | (1<<TXEN0);
  UBRR0 = UBRR_VALUE;
}



static void uart_wait(void) {
  while (!(UCSR0A & (1<<RXC0))) {}
}



static void uart_put_char(uint8_t c) {
  while (!(UCSR0A & (1<<UDRE0))) {}
  UDR0 = c;
}



static uint8_t uart_get_hex_digit(void) {
  uint8_t digit;
  uint8_t error;
  do {
    uart_wait();
    error = UCSR0A & ((1<<FE0) | (1<<DOR0));
    digit = UDR0;
    if (!error) {
      if ((digit >= '0') && (digit <= '9')) {
        uart_put_char(digit);
        digit -= '0';
        break;
      }
      else if ((digit >= 'A') && (digit <= 'F')) {
        uart_put_char(digit);
        digit = digit - 'A' + 10;
        break;
      }
      else if ((digit >= 'a') && (digit <= 'f')) {
        uart_put_char(digit);
        digit = digit - 'a' + 10;
        break;
      }
    }
  } while (1);
  return digit;
}



static void uart_put_hex_digit(uint8_t digit) {
  if (digit < 0x0A) {
    digit += '0';
  }
  else {
    digit = digit + 'A' - 10;
  }
  uart_put_char(digit);
}



static void uart_put_word(uint16_t word) {
  int8_t i;
  uint8_t digit;
  for (i=12; i>=0; i-=4) {
    digit = (word >> i) & 0x0f;
    uart_put_hex_digit(digit);
  }
}



int __attribute__ ((__OS_main__)) main(void) {

  uint8_t cmd;
  uint8_t idx;
  uint16_t word;
  
  uart_init();
  
  while (1) {

    // Get a command
    do {
      uart_wait();
      cmd = UDR0;
    } while ((cmd != 'r') && (cmd != 'R') && (cmd != 'w') && (cmd != 'W'));
    // Echo
    uart_put_char(cmd);
    
    // Get an index
    idx = uart_get_hex_digit();
    idx = (idx << 4) | uart_get_hex_digit();

    // Constrain
    if (idx < PRESET_SZ) {
      // Write
      if ((cmd == 'w') || (cmd == 'W')) {
        uart_put_char('=');
        // Get a word of data
        word = uart_get_hex_digit();
        word = (word << 4) | uart_get_hex_digit();
        word = (word << 4) | uart_get_hex_digit();
        word = (word << 4) | uart_get_hex_digit();
        // Write to EEPROM
        eeprom_update_word(&preset_ee[idx], word);
      }
      // Read
      else {
        uart_put_char('=');
        // Fetch word from EEPROM
        word = eeprom_read_word(&preset_ee[idx]);
        // Write to UART
        uart_put_word(word);
      }
    }
    uart_put_char('\r');
    uart_put_char('\n');
        
  }
    
}

 

This is a complete, self-contained test program which probes the question you posed in your OP:

Quote:
So it's gotta be the way I'm trying to use the array.
The program is interactive. Build and upload it to your device, and then open a serial terminal program and connect to your device's UART at 9600 baud (or change the #define to suit your needs).  Press 'r' to read an element from the array preset_ee[], followed by a two-digit hexidecimal index.  The value will be reported in hexidecimal.  Similarly, press 'w' to write an element to the array, followed by a two digit hexidecimal index and a four-digit hexidecimal value.

 

You'll find that it works exactly as expected.

 

Your job now is to show us a program which >>doesn't<< work as expected.  Trim it down to the bare minimum.  Forget about all of your buttons and midi code.  Show us the code which demonstrates that the eeprom functions don't work as expected.  My bet (everyone's bet) is that in the process of so doing, you will discover the real culprit, because as the above program should demonstrate, the eeprom functions do work as you expect them to.  Something else is at work here, and you must distil your code down to that problem and nothing else.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Wed. Aug 5, 2015 - 07:05 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The two important points are:

1.  always copy-paste code rather than type from scratch.

2.  provide a minimal test program that exhibits your problem.

 

Yes,  (2) looks a little daunting.    But if you do it with an Arduino,  it makes life simpler.    You can test your C functions and check that they behave as you want.     Then you can copy-paste the debugged function(s) into your 6000 line program.

 

Looking at your edited msg#1.   It all looks fine to me.    You would test it by using Serial.read() and Serial.print().   Constraining the array index to lie between 0 and 127.

 

I would be very wary of updating EEPROM from inside an ISR().   It takes several milliseconds.    Reading from within an ISR() is a very swift operation.

The main advantage of using Arduino as a test harness is that people will actually run and debug your code for you.

 

David.

Last Edited: Wed. Aug 5, 2015 - 09:10 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes,  (2) looks a little daunting. 

...but is such a good sanity check.

 

Not suitable for posting, but effective for me at times, is to take the big complete app and just hack at it to isolate.  E.g., I come across your puzzling situation in a complete production AVR app, perhaps 10s of thousands of lines.

 

The simple "sanity check" works.  So gotta be some interaction with the app.

 

Now, perhaps (as in your case) there are data structures and such that are a pain to whittle down without a zillion syntax errors for missing stuff.  But C has this marvelous statement "goto" that can be used when hacking and whacking to just short-circuit as much of the main loop as practical, and then add the problem area tests.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

theusch wrote:
C has this marvelous statement "goto" that can be used when hacking and whacking to just short-circuit as much of the main loop as practical, and then add the problem area tests.
Or, for those goto-averse, wrap the superfluous code in:

if (0) {}.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

Or, for those goto-averse, wrap the superfluous code in:

 

if (0) {}.

Indeed.  Many ways to approach it.

 

I guess my "preferred" method is to #if 0 / #endif big chunks.  But as mentioned that can sometimes cause a bunch of syntax errors with missing stuff.  On occasion, I've added another main loop before the "real" one, focusing on the problem area and moving stuff into the test loop from the real loop chunk by chunk.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Yes,   I think that I have used all of those techniques.   ....  for my private bugs.

 

If I am going to ask a question on a public forum,  it needs to be minimal,  compilable and attractive to read.

 

You will then get the most readers and several may even build your code.

 

The problem with vast chunks of "disabled" code is that a foreigner is not going to bother.    IMHO,   it is your job to do the tidying up before posting in the first place.    Which is exactly what the OP had attempted to do.

 

David.

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

@joeymorin, David, and theusch: Thank you gentlemen. All these techniques give my learning new direction.

 

Part of the problem is simply lack of knowledge on my part. Checking the .MAP file, codevision, goto statements, if(0) {}, are all new to me...even test programs. I have two tools for when things go wrong, my simple blink code (I'm sure others do this as well), and going over the code 1000's of times. Normally something jumps out. This time it did not.

 

The other part of my problem is the way I learn...I need to take on a daunting project before the individual pieces begin to make sense. Silly I know, but I can't help it.

 

david.prentice wrote:
The problem with vast chunks of "disabled" code is that a foreigner is not going to bother.    IMHO,   it is your job to do the tidying up before posting in the first place.    Which is exactly what the OP had attempted to do.

That's exactly what I was trying to do but I likely gave too much info to hide that fact. I figured, everything is pointing to this bit of code and I can't "see if something is actually being stored, so I better clarify that line of code before moving on. So, since my usage there seems to be correct (joeymorin's test program), it could be the word splitting/combining or even the default memory check.

 

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

Ok, I took everyone's suggestions and figured out a working test program below that shows the essence of the task. Problem is, the program works. sad

 

//Device: ATmega164A
#define F_CPU 8000000UL

#include <avr/io.h>
#include <avr/eeprom.h>
#include <util/delay.h>

//	Global Variables
uint16_t EEMEM midi_preset_eeprom[5];			//Used to store presets in memory
volatile uint8_t midi_data_byte_0 = 4;			//Gets first MIDI Data Byte

int main(void)
{
	//Initialize PortA to check results
	DDRA = 0xB0;					//Sets up Data Direction Register A, PinA4, PinA5 and PinA7 as outputs.
	PORTA = 0xFF;					//Sets all PORT A pins 5V.

	uint8_t activeH = 125;			//Temp variable to shift 125 into midi_preset
	uint8_t activeL = 73;			//Temp variable to shift 73 into midi_preset
	uint16_t midi_preset = 0;		//Used to read and write the current midi_preset
	uint8_t midi_preset_number = 0; //Used to store midi preset in correct location in memory
	
	//Store a preset
	midi_preset_number = midi_data_byte_0;					  //store program change data as midi preset number
		
	midi_preset = ((activeH & 0xFF) << 8) | (activeL & 0xFF); //shifts active into MSB and active_model into LSB

	eeprom_update_word(&midi_preset_eeprom[midi_preset_number], midi_preset); //writes midi_preset to EEPROM

	//Change active variables from saved values to simulate normal use
	activeH = 22;
	activeL = 143;

	//recall a preset
	midi_preset = eeprom_read_word(&midi_preset_eeprom[midi_preset_number]); //Gets the preset data from eeprom for the received preset number (midi_data_byte_0)
		
	//Parse word into 2 bytes
	activeH = ((midi_preset >> 8) & 0xFF);		//Shifts activeH portion of m_p_r into m_p_a
	activeL = (midi_preset & 0xFF);				//Shifts activeL portion of m_p_r into m_p_a
			
	if((activeH != 0xFF) && (activeL != 0xFF))		//Checks to see if there is MIDI Data stored here, default eeprom is 0xFF, 0xFF is not valid config
	{
		if ((activeH == 125) && (activeL == 73))	//Checks to see if our values came back correct
		{	
			//If so, blink for success
			PORTA ^= 1<<PINA5;
			_delay_ms(500);
			PORTA ^= 1<<PINA5;
			_delay_ms(500);
			PORTA ^= 1<<PINA5;
			_delay_ms(500);
			PORTA ^= 1<<PINA5;

		}

	}
}

This program follows the same steps I described with the first version I posted. I've just hard coded some values and checked for those values. So I guess this is good, it eliminates the array storage, the shifts, and the default check as culprits and points to:

 

theusch wrote:
I know you said you verified that preset_number was set correctly but as a guess it ain't what you think it should be.  (>>if<< your posted fragment in the OP actually represents the real app code)

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

Problem is, the program works. sad

Actually, as this is a sanity check, it is really a :)  smiley as we are now all proven to be still sane.

 

After the sanity check passes, then there are usually two painful choices:  Add chunks at a time to the test program until it again shows the symptoms.  Or go back to original code and start disabling chunks until it starts working.

 

 

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

Now, as you are spending much effort on this situation, another suggestion would be to set up the JTAGICE3 as JTAG ICE and then you can get to the trouble spot in the app with a breakpoint, and examine stuff.  (hoping that the JTAG pins aren't a critical part of the test program)

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

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

theusch wrote:
Actually, as this is a sanity check, it is really a :)  smiley as we are now all proven to be still sane.

I wish there was a "like" button for this, made me laugh.

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

Run it in the studio simulator. You can step through your code and try to observe when things go wrong.

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

Ok, sanity check 2 works as well. But this is really bad.

//Device: ATmega164A
#define F_CPU 8000000UL

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

//Defines USART Baud Rate
#define USART_BAUDRATE 31250
#define BAUD_PRESCALE (((F_CPU / (USART_BAUDRATE * 16UL))) - 1)

#define MIDI_STATUSBYTE_MSK 0x80				//Used to check if data received is a status byte (AND with data)
#define MIDI_CHANNEL_MASK 0x0F					//Used to check if data received comes in on correct channel
#define PROGRAM_CHANGE 0xC0						//Used to check if data received is a program change message

#define CLEAR_MIDI_STATUS_BYTE	0x00			//Sets status byte MSB = 0 (invalid MIDI status byte)
#define CLEAR_MIDI_DATA_BYTE	0x80			//Sets data byte MSB = 1 (invalid MIDI data byte)

//	Global Variables
uint16_t EEMEM midi_preset_eeprom[128];			//Used to store presets in memory

volatile uint8_t midi_status_byte = CLEAR_MIDI_STATUS_BYTE;			//Gets MIDI Status Byte
volatile uint8_t midi_data_byte_0 = CLEAR_MIDI_DATA_BYTE;			//Gets first MIDI Data Byte
volatile uint8_t last_midi_byte_was_data = 0;						//Used to ignore second data byte
volatile uint8_t program_change_received = 0;						//Used to tell the ISR and software a program change was received

//Can configure for testing
volatile uint8_t midi_channel = 0;				//Which MIDI channel are we listening to?
volatile uint8_t omni_on = 1;					//If 1, omni mode is on

int main(void)
{
	//Initialize PortA to check results
	DDRA = 0xB0;					//Sets up Data Direction Register A, PinA4, PinA5 and PinA7 as outputs.
	PORTA = 0xFF;					//Sets all PORT A pins 5V.

	//	USART Initialize
	UBRR0H = (unsigned char)(BAUD_PRESCALE >> 8);	//Load upper 8-bits of the baud rate value into high byte of UBRR0 register
	UBRR0L = (unsigned char)(BAUD_PRESCALE);		//Load lower 8-bits of the baud rate value into low byte of UBRR0 register
	UCSR0B = (1<<RXEN0) | (1<<RXCIE0);				//Enable UART receiver and enable USART Recieve Complete interrupt (USART_RXC)
	UCSR0C = (1<<UCSZ00) | (1<<UCSZ01);				//Use 8-bit message sizes (asynchronous, 1bit stop, no parity) MAY NEED TO WRITE TO URSEL on other AVR's

	sei();
		
	
	
	for(;;)
	{
		uint8_t activeH = 125;			//Temp variable to shift 125 into midi_preset
		uint8_t activeL = 73;			//Temp variable to shift 73 into midi_preset
		uint16_t midi_preset = 0;		//Used to read and write the current midi_preset
		uint8_t midi_preset_number = 0; //Used to store midi preset in correct location in memory

		//Wait for preset number from USART to store  
		while(program_change_received == 0)						  //Wait for a program change message
		while (midi_data_byte_0 == CLEAR_MIDI_DATA_BYTE)		  //wait for MIDI data byte
		midi_preset_number = midi_data_byte_0;					  //store program change data as midi preset number
		
		midi_preset = ((activeH & 0xFF) << 8) | (activeL & 0xFF); //shifts active into MSB and active_model into LSB

		eeprom_update_word(&midi_preset_eeprom[midi_preset_number], midi_preset); //writes midi_preset to EEPROM
		
		midi_preset_number = 0;									  //Clear midi_preset number and wait for new one
		program_change_received = 0;							  //Clear program change to wait for next
		midi_status_byte = CLEAR_MIDI_STATUS_BYTE;
		midi_data_byte_0 = CLEAR_MIDI_DATA_BYTE;				  //Clear midi_data_byte_0 to wait for next

		//Change active variables from saved values to simulate normal use
		activeH = 22;
		activeL = 143;

		//Wait for a preset number from USART to recall 
		while(program_change_received == 0)						  //Wait for a program change message
		while (midi_data_byte_0 == CLEAR_MIDI_DATA_BYTE)		  //wait for MIDI data byte
		midi_preset_number = midi_data_byte_0;					  //store program change data as midi preset number

		midi_preset = eeprom_read_word(&midi_preset_eeprom[midi_preset_number]); //Gets the preset data from eeprom for the received preset number (midi_data_byte_0)
		
		
		//Parse word into 2 bytes
		activeH = ((midi_preset >> 8) & 0xFF);		//Shifts activeH portion of m_p_r into m_p_a
		activeL = (midi_preset & 0xFF);				//Shifts activeL portion of m_p_r into m_p_a
			
		if((activeH != 0xFF) && (activeL != 0xFF))		//Checks to see if there is MIDI Data stored here, default eeprom is 0xFF, 0xFF is not valid config
		{
			if ((activeH == 125) && (activeL == 73))	//Checks to see if our values came back correct
			{	
				//If so, blink for success
				PORTA ^= 1<<PINA5;
				_delay_ms(500);
				PORTA ^= 1<<PINA5;
				_delay_ms(500);
				PORTA ^= 1<<PINA5;
				_delay_ms(500);
				PORTA ^= 1<<PINA5;

			}

		}

		midi_preset_number = 0;									  //Clear midi_preset number and wait for new one
		program_change_received = 0;							  //Clear program change to wait for next
		midi_status_byte = CLEAR_MIDI_STATUS_BYTE;
		midi_data_byte_0 = CLEAR_MIDI_DATA_BYTE;				  //Clear midi_data_byte_0 to wait for next
	}/* end for */
}

ISR(USART0_RX_vect)
{
	uint8_t received_byte;			//Temp variable to process UDR data
	
	received_byte = UDR0;			//Store received MIDI data into recieved_byte for further processing
	
	//if recieved_byte is status byte
	if ((received_byte & MIDI_STATUSBYTE_MSK) == MIDI_STATUSBYTE_MSK)
	{
		//if received byte is program change message
		if ((received_byte & PROGRAM_CHANGE) == PROGRAM_CHANGE)
		{
			if (((received_byte & MIDI_CHANNEL_MASK) == midi_channel) || (omni_on == 1))
			{
				program_change_received = 1;			//set program change received variable to 1
				
				midi_status_byte = received_byte;		//otherwise, midi_status_byte0 is ready for data
			}
			
		}//otherwise ignore data until program change received we need to listen to
		
		last_midi_byte_was_data = 0;			//Last byte was a status byte
	}
	else
	{
		//if we have received a program change
		if (program_change_received == 1)
		{
			//check to see if byte received was a data byte
			if (last_midi_byte_was_data == 0)			//If last midi byte was a status byte we need to store data
			{
				midi_data_byte_0 = received_byte;		//store data in midi_data_byte_0
			}

			last_midi_byte_was_data = 1;				//last MIDI byte was data, ignore until new status byte

		}
		//otherwise ignore data until program change received that we need to listen to
	}
	
}

Now I've added back the ISR receive code so I can send the preset number from my computer via MIDI. The test program waits in a while loop for the preset number to be sent and stores the hard-coded data in that memory location. The program changes the data to simulate normal use, then waits in while loop again for the next preset number from the computer. It then reads the memory location at that address and checks for the hard-coded data. If it is received, and LED blinks twice. It blinks when the right preset number is sent, doesn't when the wrong is sent.

 

This is horrible because it mimics my code almost exactly, which still doesn't work. In the non-working version, I (think I) verified that no preset is being stored by attempting to recall every preset number. Other than that, the only differences that I can see are the use of separate functions in the real code and that the activeH and activeL variables aren't hard coded. But they keep a running status of the unit, so they have to be right.

 

I have no clue what is happening here.

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

Kartman wrote:

Run it in the studio simulator. You can step through your code and try to observe when things go wrong.

I would but I don't know the feasibility since the unit is purpose built, it relies on a lot of user input, and it's over 5k lines of code!

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

This smells more and more like one (or more) of the following:

 

  • uninitialised automatic variable
  • stack overflow
  • buffer overrun or underrun
  • bad pointer

 

These can be hard to track down.  A debugger can help.  It may be something else entirely.  Either way, as suggested, you'll need to either a) start adding code back in chunks, or b) go back to the non-functioning code and start removing chunks.

 

This will be a lot easier if you designed and coded in a structural, modular way.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

@joeymorin: YOU DID IT!!!! I don't know what or how, but you did it!

 

Actually, you all did. If it was not for each one of you I would not have likely stumbled on this. Lots of steps but you kept pushing and challenging me in the right direction. Thank you sincerely, each and every one of you who took the time out to help. I'm elated.

 

Before I go, can I ask one more thing? What was going on?

 

I followed JJ's advice, checked for initialized local variables, there were none. I added bounding in the ISR to keep the midi_data_byte_0 (which turns into midi_preset number) to keep it between 0-127 (buffer overrun/underrun), didn't help. Then finally, I addressed the local variables (stack issue?).

 

Each function, store and restore, needed 4 variables: the preset number, the preset data, then two bytes to get and set the current active state of the unit. For whatever reason, I made them local and unique (you can see what I'm talking about in the code in post #21 below //local variables) which should be fine since the data isn't shared between functions. But because they do the same thing I figured I could make them global, being sure to clear them after use, and turn 8 variables into 4. That's what made it work. So why, or at least what kinds of things could be happening because of that simple change?

 

THANK YOU joeymorin, meslomp, clawson, theusch (sanity checks, I was beginning to wonder), david.prentice and Kartman! I really appreciate your help. 

 

 

Last Edited: Thu. Aug 6, 2015 - 05:46 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are talking about these?

 

uint8_t validate_Midi_Store (void)
{
	//Local Variables
	uint8_t activeH = 0;			//Temp variable to shift data into midi_preset
	uint8_t activeL = 0;			//Temp variable to shift data into midi_preset
	uint16_t midi_preset = 0;		//Used to read and write the current midi_preset
	uint8_t midi_preset_number = 0; //Used to store midi preset in correct location in memory

 

I'm not clear - are you saying they work as locals and fail as globals or the other way round?

 

The main difference between local and global is the position in memory where they are created. globals (with 0 values) will be in the .bss area which are fixed locations allocated at link time. In the AVR RAM the .data section is placed at the start of RAM followed by the .bss. In the case of locals they are automatics created on the stack on entry to the function (and lost when it returns). GCC starts the stack at the end of RAM then it works backwards.

 

A couple of things might be occurring:

 

1) a rogue pointer just happens to hold a value that is somewhere near the stack (or the .bss) and when it's written through this corrupts the variables - the affect is only noticed in one of the two locations because that's where the pointer happens to be pointing

 

2) the variables are located just after an array/buffer that is over-running. This scenario could only be the problem for the case where they are global. The variable/array placed immediately before them is written beyond its bounds and the write corrupts these variables that are innocently sitting right after it.

 

EDIT: re-reading your post I think I understand that you are saying global=works, local=fails. So it's when they are at the base of the stack that the problem occurs? That sounds a lot like an interrupt may be occurring and somehow corrupting them. Do you have ANY code in this program written in Asm rather than C? Especially ISR code.

Last Edited: Thu. Aug 6, 2015 - 09:33 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you, clawson!

clawson wrote:
EDIT: re-reading your post I think I understand that you are saying global=works, local=fails. So it's when they are at the base of the stack that the problem occurs? That sounds a lot like an interrupt may be occurring and somehow corrupting them. Do you have ANY code in this program written in Asm rather than C? Especially ISR code.

This is right, global works, local fails. No nothing is Asm, just C. We never learned Asm except for the occasional brief.

 

It's interesting that you say an interrupt may be occurring because it very well could be. The standard MIDI message is 2 or 3 bytes long, one status byte with MSB=1, and two data bytes with MSB=0. We are only listening for a program change message which is two bytes long.

 

The ISR reads the data, checks if it is something we care about (program change msg and if the unit is set to respond to this message) and if we received a valid status byte, a flag is set to trigger an if statement in the main loop. But before we do anything we need the associated data byte, which too will come from the ISR, so there's a while loop to wait for it. I did this to prevent the situation where the main loop processes to fast where it enters the process MIDI code, but there's no data byte yet. If the data byte is there already, then the while doesn't matter. 

 

But there's two situations that could cause another interrupt to fire somewhere in the processing:

 

1. Another MIDI message is sent right away (which could very well happen if a lot of program change messages need to be sent over the buss at once), or

2. The transmitting software sends a blank 3rd byte for every program change. To prevent the ISR from getting a 2nd data byte and trying to overwrite the important one, I test for and only allow the first data byte to be processed.

 

Oh but wait, what would happen if the ISR fires while initializing the local variables? The local initialization was happening between the program change received flag and the preset processing. That could explain why declaring them globally makes the difference, no?

Last Edited: Thu. Aug 6, 2015 - 05:06 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

It's interesting that you say an interrupt may be occurring because it very well could be.

As long as it's all C it shouldn't matter. The C compiler knows what state/register it changes and it saves/restores all that are necessary (always). It's when people dabble in Asm they maybe don't spot the consequence of something they are doing in an ISR and it's actually hitting the stack or whatever which might explain the damage.

 

So an interrupt occurring alone should have no detrimental effect. Of course you can do perverse things like:

ISR(something) {
    SP += 2;
}

or

ISR(something) {
    char * p = rand();
    *p = 0x55;
}

and that kind of thing could seriously ruin your day.

 

But if the locals really are getting corrupted within that function (and not by anything it calls) then the likely suspect is something that is going on in an ISR as it's the only way something could "get in" and do damage between the point where the variables are created/initialised and where they are used.

 

A JTAG debugger would help find things 1,000,000 times quicker than anything else!

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

Ok, I don't think I'm doing anything perverse in the ISR, just a few comparisons to figure out what the incoming data is and setting a few flag variables to control main.

 

clawson wrote:
A JTAG debugger would help find things 1,000,000 times quicker than anything else!

Learning how to do this and learning how to write code in a more structural, modular way are both on my to-do list. smiley

 

I'll keep testing but thank you again for all of your help!

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

If an automatic local array overruns, it may clobber something on the stack. If this (or something like it) is what's happening, moving an automatic to .bss or .data will either a) move it out of harms way, or b) if the thing that was moved is the thing that is overrunning then it now no longer poses a threat to the stack. Neither of these scenarios is a solution.
If you're dealing with 6000+ lines of non-modular code, then you've got you work cut out for you. And there's of course no guarantee that we're on the right track.
Return to using locals for thos variables, and start looking for other things you can do to 'fix' the issue. If changing something else can also 'disappear' the problem, that might be a common clue as to what is at the root.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

joeymorin wrote:
Return to using locals for thos variables, and start looking for other things you can do to 'fix' the issue. If changing something else can also 'disappear' the problem, that might be a common clue as to what is at the root.

Ok, I'm on it.

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

joeymorin wrote:
If an automatic local array overruns, it may clobber something on the stack. If this (or something like it) is what's happening, moving an automatic to .bss or .data will either a) move it out of harms way, or b) if the thing that was moved is the thing that is overrunning then it now no longer poses a threat to the stack. Neither of these scenarios is a solution.

Just for clarification, declaring a local variable static basically does the same thing, and is also not a "solution", correct?

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

Sothis wrote:
Just for clarification, declaring a local variable static basically does the same thing, and is also not a "solution", correct?
Correct.  A static variable (or struct, array, ...) goes into .bss if it is not initialised (i.e. C initialises it at as zero), or .data if it is.  All globals are static.  A local is automatic unless specified with the static keyword.

 

The 'not a solution' part is because neither approach fixes the (presumed) underlying issue of a buffer overrun.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Thu. Aug 6, 2015 - 09:16 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

So guys, I've been at this for days and nothing is jumping out.

 

I've shortened the interrupt and created an atomic buffer to eliminate corruption there, I've looked for uninitialized variables and arrays (only array is EEMEM midi_preset[128] and now my buffer). No pointers. Started eliminating code and trying to make it more modular.

 

Interestingly, I noticed a new symptom. There's two places we need to check the usart data, main and and second function that loops as well (this is where we get the preset number to store the data at). I condensed the code so that both main and the storage function call the new "process_Midi" function, which reads data from the buffer and determines if we have a program change or not (it's actually the old ISR code).

 

In the main loop, when I send a valid program change, the code rarely recognizes the data as a program change but it does sometimes. However, if I'm in the alternate loop, the program change is recognized 100% of the time.

 

I'm wondering, could all this just be that I need a bigger ATmega? I'm at 91.2% program memory usage, 5.9% data (mostly global variable), and 52.3 EEPROM. There's a a lot of repeated code (but with new variable names) that can likely be turned into functions so the repeats are unnecessary, and potentially an opportunity to both reduce global variable usage. I might be able to further reduce global variable usage in place of more local variables. Does modular programming  = more but smaller functions? Are these things going to potentially help?

 

Beyond this, I have no clue. 

Last Edited: Sun. Aug 9, 2015 - 05:34 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Program memory usage is not the issue.  Neither is EEPROM.  The 5.9% reported for data only includes static usage i.e. .data and .bss, >>not<< automatics, >>nor<< other stack usage.  Your '164A has 1024 bytes of SRAM.  If 5.9% is used, that's about 60 bytes, leaving 964 bytes for stack use.  It's quite unlikely that your program uses that much, unless you've got large automatic arrays.  Running out of SRAM is not the likely scenario.  Trampling on something in SRAM is much more likely.

 

Yes, modularising generally means more and smaller functions.  Each main component of your app is implemented using one or a few simple functions, which themselves are built on a small number of simple functions, etc.  Modularising your code will make it easier to track down a problem.  By modularising your code, you can swap out any given function for a non-functioning (or minimally functioning, or 'skeleton', or...) until the problem 'disappears'.

 

While it is not always the case that the swapped-out function is the one responsible for the problem, it is a powerful tool for debugging.  It is also an important part of development in the first place.  You develop a module in isolation, often within the context of a test program or test 'harness'.  Write it, debug it, lock it down.  Move on to the next module.  Once all the modules are working independently, you start to put them together and test/debug their interaction, two at a time.  Only once you've locked down all of these steps should you consider putting them all together.

 

But, >>before<< you do >>any<< of this, sit down with a pencil and paper and >>design<< the app.  I've mentioned it early in this thread: “ Without requirements or design, programming is the art of adding bugs to an empty text file.  This may be a hard pill to swallow after having spent so much time writing and testing your app up to this point.  It may indeed be faster to just knuckle down and brute-force your way to a solution for this one problem.  However, that won't help you when the next unexpected problem pops up.  And don't kid yourself, it will.  Sometimes it's better to start over.  Only you can decide if that is the case here.  You seem to have done some some work to modularise your existing code, but that may prove more difficult than going back to the drawing board.  You may still be able to use already-written code, just be sure to do so with the above principles in mind.  Don't take anything for granted.  Test fully in isolation before moving on to the next step.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Sun. Aug 9, 2015 - 07:58 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@joeymorin, sage advice. The worst part of this is the unit was working perfectly. It's only after adding the ISR and the code to process it that we've had problems. Ironically, the parts that were working continue to appear to work flawlessly. Anything besides arrays and pointers that aren't so obvious that would trample SRAM? 

 

Thanks again JJ, I really appreciate your help!

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

Quote:
Anything besides arrays and pointers that aren't so obvious that would trample SRAM?
Plenty, but those are the most likely suspects.

 

It's easy to overlook something which is a pointer.  Any function arguments which are pass-by-reference are by definition a pointer.  Any 'C' string is an array.  All arrays are reference by means of a pointer.

 

It may not be SRAM getting clobbered, either, but may instead be something like a function pointer getting corrupted.  This assumes of course that you're using function pointers.  'Rogue' code execution may nevertheless appear to 'work', but behave strangely.  Even a 'software reset' (caused sometimes by an unhandled interrupt, or a null function pointer) may in certain circumstance appear to 'work', if 'strangely'.

 

Perhaps you could post your ISR code in its entirety, and the main-thread code which processes it.  Include declarations/definitions for any related variables, arrays, or functions.  I realise that this will not be a 'complete' statement of the problem, but perhaps it will be enough for someone to give you a clue.

 

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

  Running out of SRAM is not the likely scenario. 

Just checking - this app does not contain any use of "malloc()" does it?

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

'The unit was working perfectly' - how many times have I been caught by that one! Sometimes things work but for the grace of God. Once you find the root cause, I've found that the defect was there all along, but hidden. Version control systems allow you to drill back to when the defect was introduced and who to blame. Moi? Never!

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

clawson wrote:
Just checking - this app does not contain any use of "malloc()" does it?

No, no malloc(). No strings, function pointers, or tricks for that matter...just a lot of basic stuff and bitwise operations.

 

Kartman wrote:
'The unit was working perfectly' - how many times have I been caught by that one! Sometimes things work but for the grace of God. Once you find the root cause, I've found that the defect was there all along, but hidden. Version control systems allow you to drill back to when the defect was introduced and who to blame. Moi? Never!

Oh I'm positive I'm to blame, haha. I just can't find where I'm to blame. I know computers 99.999% of the time don't just not work, they appear to not work because they are doing exactly what they've been told. 

 

Each time you guys take time to post, you give me new ideas of things to look for, so thank you again. I know we'll solve this. smiley

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

Looks like I found the solution. In removing chunks of code, I would comment out the functions and the function calls in main which didn't yield a result. After joeymorin suggested I post the ISR and main, I realized I hadn't considered a sub-function in main to be the issue. With everything but the ISR data checking code in main commented out, it worked. I added code back in until the very last sub-function where it stopped working again. A check revealed the code to always be executing, calling the i2c bus 2x every pass when the code should only happen in rare occasions because PIND3 wasn't tri-stated like it should be, so it was always set.

 

Below is the bad sub-function:

 

if (bit_is_set(PIND, 3) && (ftsw_unplugged == 1)) //Should be "if((bit_is_set(PIND, 3)) && (ftsw_unplugged == 1))
{
    _delay_ms(10);
            
    //Configure Footswitch 
    ftsw_unplugged = i2c_start(FTSW_IO_ADDR + I2C_WRITE);	//Sends a start command followed by FTSW Cntlr address and write
    if (ftsw_unplugged == 0)					//If the ftsw is detected, configure it.
    {
        i2c_write(PCA9534_CONFIG_PORT_CB);		//Sends command byte that says we are writing to the Config PORTs
        i2c_write(0x70);						//Write to Config PORT pins 4, 5, 6 inputs
    }
    i2c_stop();
            
    //Write to Foootswitch
    ftsw_unplugged = i2c_start(FTSW_IO_ADDR+I2C_WRITE);	 //Issues a START Condition and sends the FTSW CNTLR address with a write bit
    if (ftsw_unplugged == 0)
    {
        i2c_write(PCA9534_OUT_PORT_CB);			//Sends the CMD Byte to the FTSW controller that we are accessing the Out Port
        i2c_write(~current_Ftsw_Port);			//Writes the 1's compliment of current_Ftsw_Port to Ftsw Output Port
    }
    i2c_stop();
}

So for learning sake, what about the i2c bus would corrupt data from the ISR or the atomic buffer holding the data from the ISR? Also, for a modular approach, would we make this its own function called by main as opposed to a block within?

 

EDIT: added info about what was fixed.

Last Edited: Tue. Aug 11, 2015 - 01:30 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
if (bit_is_set(PIND, 3) && (ftsw_unplugged == 1))

How did that even compile?

 

[EDIT: nevermind ;) ... my eyesight is failing... thought there weren't enough parentheses.]

 

I'd be suspicious of losing data on the MIDI bus.  If you say that the above code snippet was running every time through, then it would run at most 100 times per second, because of the _delay_ms(10).  Hardly enough to handle MIDI data which comes in at 31250 baud (3125 bytes per second).

 

Quote:
what about the i2c bus would corrupt data from the ISR or the atomic buffer holding the data from the ISR?
It couldn't.  The >>software<< can do that, though.  What I2C library are you using?  Does it disable interrupts?  How are you handling the MIDI data coming in via the UART?  Do you have a ring buffer to handle bursts of data?  How big is the buffer?

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Mon. Aug 10, 2015 - 11:02 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

joeymorin wrote:
It couldn't.  The >>software<< can do that, though.  What I2C library are you using?  Does it disable interrupts?  How are you handling the MIDI data coming in via the UART?  Do you have a ring buffer to handle bursts of data?  How big is the buffer?

The i2c library is the Fleury driver, which doesn't appear to disable interrupts.

 

Before, I was processing the MIDI data directly in the ISR since there's only two things we care about, a program change message for our channel, and the data byte. The rest is noise. But I thought that that processing might be taking too long to and was the problem, so I moved the processing to its own function and inserted a simple ring buffer from code found online. That code, shown below calls the buffer directly from the ISR, but calling a function concerned me, so I just put the buffer code in the ISR:

 

ISR(USART0_RX_vect)
{
	//cli();
	uint8_t data = UDR0;

	usartBuffer[writeIndex] = data;
	writeIndex++;

	if(writeIndex >= BUFFERSIZE)
	{
		writeIndex=0;
	}
	//sei();
}

The buffer size started at 10, but I have it at 3 now since I was concerned about stack usage. I'll likely increase that to 12 to 15 if testing keeps going well. No incidences since fixing the main sub-function.

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

What did you 'fix'?

 

Your ring buffer doesn't handle the case where the buffer is full, wherein it will overwrite the oldest unread entry.  Are you >>certain<< you're not losing bytes?

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

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

joeymorin wrote:
What did you 'fix'?

Ah, sorry! I got burned by the back button again. FORGOT: I found that PIND3 was initialized to 5V instead of tri-stated like it should have been, so the i2c code above was always triggering. 

 

No, I'm not certain I'm not losing bytes at this point. I have only been sending one shot program change messages (why I chose 3 for the buffer size) and that wasn't working until fixing the PIND3 issue. Now the process_MIDI code (old ISR) sees a program change every time. All I really know at this time is that the PIND3 code above seems to be the thing causing the bad behavior. Why is still eluding me.

 

I wouldn't think the buffer would be the issue when it does the same when the buffer size is 10 or 3, and only in main (next to the i2c code).

Last Edited: Tue. Aug 11, 2015 - 01:11 AM
This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Take a fresh look (if that's even possible now wink) at the rest of your code with something new in mind:  what would happen to it if >>every<< pass through your main loop (or whatever part of your code executes the if (bit_is_set(PIND, 3) block) incurred a 10 ms busy loop?

 

I expect you will find edge cases which are not handled, buffer-full conditions which are not tested (like in your ISR), counter variables exceeding expected values, things of that nature.

 

Start making all of your code bullet-proof.  Test for conditions which you think are impossible.  Always keep in mind what could go wrong >>if<<...

 

EDIT: When you trap an unexpected condition, flag it to the outside world somehow.  Light an LED.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"Wisdom is always wont to arrive late, and to be a little approximate on first possession."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Tue. Aug 11, 2015 - 02:50 AM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Ya, you are right 100%. The funny thing is I forgot to mention that I started suspecting counter variables so I attempted to harden them a bit prior to stumbling on this error. Also, I had thought that I drilled my button holes too small and they were getting stuck down...but now they don't exhibit that same behavior anymore. Unbelievable...and totally believable at the exact same time.

 

As you said, I've been taking a fresh look at this code ever since you mentioned "structured, modular programming". Really, I was looking for an error, but it changed the way I thought about the code that is there. Assuming I understand what that is (I've been reading up), there's a TON of opportunity to modularize this code. I'd think getting the program memory usage from 90% down to below 60% is a very real possibility.

 

Thank you so much to everyone, especially you JJ. I only came to learn about accessing arrays in EEPROM, but I'm walking away with a ton more that I've learned and more opportunities to guide future learning. That is far more valuable. wink