Understanding counting with arrays

Hello all,

I got my first UNO a few weeks ago and have been managing my way thru various projects here and there. One of which was to incorporate some LEDs into some toy helicopters for flying at night -- big thanks to mr. klein! Here's a video of how things turned out: S107 + 50hp mod - YouTube

I built a socket into the project to allow for adding more patterns later and this is where i'm running into some trouble: if i add too many entries to my array, the program fails to operate correctly.

Here's another vid to further illustrate the problem: counting with arrays - YouTube

The first 25 seconds show normal operation while the latter part of the video shows what happens after uncommenting the line in the brightness_pattern array. It doesn't matter which line is commented out in the array, 7 entries is okay but 8 is too many.
FWIW, I originally wrote the program with an UNO board but then trimmed it down for use on an ATTiny44 at 8MHz

The code:

//
//	www.blinkenlight.net
//
//	Copyright 2011 Udo Klein
//
//	This program is free software: you can redistribute it and/or modify
//	it under the terms of the GNU General Public License as published by
//	the Free Software Foundation, either version 3 of the License, or
//	(at your option) any later version.
//
//	This program is distributed in the hope that it will be useful,
//	but WITHOUT ANY WARRANTY; without even the implied warranty of
//	MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
//	GNU General Public License for more details.
//
//	You should have received a copy of the GNU General Public License
//	along with this program. If not, see http://www.gnu.org/licenses/


#include <EEPROM.h>

uint8_t get_next_count(const uint8_t count_limit) {
	// n cells to use --> 1/n wear per cll --> n times the life time
	const uint16_t cells_to_use = 256;

	// default cell to change
	uint8_t change_this_cell  = 0;
	// value of the default cell
	uint8_t change_value = EEPROM.read(change_this_cell);

	// will be used to aggregate the count_limit
	// must be able to hold values up to cells_to_use*255 + 1
	uint32_t count = change_value;

	for (uint16_t cell = 1; cell < cells_to_use; ++cell) {
		uint8_t value = EEPROM.read(cell);

		// determine current count by cummulating all cells
		count += value;

		if (value != change_value ) {
			// at the same time find at least one cell that differs
			change_this_cell = cell;
		}
	}

	// Either a cell differs from cell 0 --> change it
	// Otherwise no cell differs from cell 0 --> change cell 0

	// Since a cell might initially hold a value of -1 the % operator must be applied twice
	EEPROM.write(change_this_cell, (EEPROM.read(change_this_cell) % count_limit + 1) % count_limit);

	// return the new count
	return (count + 1) % count_limit;
}

uint8_t brightness_pattern_1(const int8_t led, const int8_t pos) {
	switch (abs(led-pos+2)) {
		case 0: 	return 40;
		case 1: 	return 20;
		case 2: 	return 10;
		case 3: 	return 1;
		default:	return 1; //brightness
	}
}

uint8_t brightness_pattern_2(const int8_t led, const int8_t pos) {
	switch (abs(led-pos+2)) {
		case 0: 	return 40;
		case 1: 	return 20;
		case 2: 	return 1;
		case 3: 	return 0;
		default:	return 0; //brightness
	}
}

uint8_t brightness_pattern_3(const int8_t led, const int8_t pos) {
	switch min(abs(led-pos-1),abs(10-led-pos-1)) {
		case 0: 	return 40;
		case 1: 	return 20;
		case 2: 	return 10;
		case 3: 	return 1;
		default:	return 1; //brightness
	}
}

uint8_t brightness_pattern_4(const int8_t led, const int8_t pos) {
	switch min(abs(led-pos-1),abs(10-led-pos-1)) {
		case 0: 	return 40;
		case 1: 	return 20;
		case 2: 	return 1;
		case 3: 	return 0;
		default:	return 0; //brightness
	}
}

uint8_t brightness_pattern_5(const int8_t led, const int8_t pos) {
	int8_t tmp = 2*pos - abs(2*led-10)-2; //last number is middle delay
	return (tmp>0? tmp+tmp>>1: 1); //brightness
}

uint8_t brightness_pattern_6(const int8_t led, const int8_t pos) {
	int8_t tmp = 2*pos - abs(2*led-10)-2; //last number is middle delay
	return (tmp>0? tmp+tmp>>1: 0); //brightness
}

uint8_t brightness_pattern_7(const int8_t led, const int8_t pos) { // inverse of brightness_pattern_8
	int8_t tmp = abs(2*led-10) + 8 - pos; // abs(2*led-[#LEDs - 1]) + delay in middle - pos - delay at outside
	return (tmp>0? tmp+tmp>>1: 0); //brightness
}

uint8_t brightness_pattern_8(const int8_t led, const int8_t pos) { // inverse of brightness_pattern_7
	int8_t tmp = abs(2*led-10) + 25 - 2*pos; // abs(2*led-[#LEDs - 1]) + delay in middle - pos - delay at outside
	return (tmp>0? tmp+tmp>>1: 0); //brightness
}

typedef uint8_t (*brightness_pattern)(const int8_t, const int8_t);

brightness_pattern pattern[] = {  brightness_pattern_1,
				  brightness_pattern_2,
				 // brightness_pattern_3,         *************the problem seems to be here in this array*************
                                  brightness_pattern_4,
                                  brightness_pattern_5,
                                  brightness_pattern_6,
                                  brightness_pattern_7,
                                  brightness_pattern_8,
                                };

brightness_pattern brightness;

void setup() {
    
	for (uint8_t pin=0; pin<11; ++pin) {
		pinMode(pin, OUTPUT);	
	}

	brightness = pattern[get_next_count(sizeof(pattern)/sizeof(pattern[0]))];
}

void pulse_width_modulation(const uint8_t pos) {
	for(uint8_t times=0; times<8; ++times) { 
		for (uint8_t pass=0; pass<40; ++pass) { //brightness on or off...overall brightness (flicker)
			for (int8_t led=-3; led<11; ++led) {
				digitalWrite(led, (brightness(led, pos) > pass));
			}
		}
	}
}

void loop() {
 
  // wait 2 milliseconds before the next loop
  // for the analog-to-digital converter to settle
  // after the last reading:
  //delay(2);            
  
	static uint8_t pos=0;

	while(pos<16) {
		pulse_width_modulation(pos);
		++pos;
	}

	while(pos>0) {
		--pos;
		pulse_width_modulation(pos);
	}
}

thanks!

	uint8_t change_this_cell  = 0;
	// value of the default cell
	uint8_t change_value = EEPROM.read(change_this_cell);

How is one supposed to make a connection between a variable called change_this_cell and an address in EEPROM? Meaningful names would make your code easier to follow.

	uint8_t change_this_cell  = 0;
	for (uint16_t cell = 1; cell < cells_to_use; ++cell) {
			change_this_cell = cell;

Storing a uint16_t in a uint8_t is going to potentially require a big shoe horn. You really need to evaluate all the argument types to see whether they are all reasonable.

	for(uint8_t times=0; times<8; ++times) { // pot input divide by 25 or 30 for Leonardo....multiply by 5 for Uno

The relationship between the comment and the code is???

i'm sorry. this code isn't mine...it is from the blinkenlight blog that is linked from the arduino playground:

about your last question: that comment was for when i had a potentiometer connected for speed adjustment...when i shrank the project down, i had to get rid of the pot

when i shrank the project down, i had to get rid of the pot

But you couldn't delete the comment?

i'm sorry. this code isn't mine

You stole it. You modified it. It IS now yours.

You are saying that since you didn't invent it, you don't have any requirement to understand it, to improve it, or to debug it. That attitude is not acceptable around here.

PaulS:

when i shrank the project down, i had to get rid of the pot

But you couldn't delete the comment?

i'm sorry. this code isn't mine

You stole it. You modified it. It IS now yours.

You are saying that since you didn't invent it, you don't have any requirement to understand it, to improve it, or to debug it. That attitude is not acceptable around here.

excuse me?

you're right. as i stated in my original post, i am new to this.

i posted a comment on the original authors blog and he pointed me here. i came here hoping for help with my code, not to be instigated by you. if you read the title of my post, it starts with the word "Understanding," so at this point you are just putting words in my mouth.

I'm not sure why you're accusing me of stealing; that's quite out of line. It clearly states that it is free for modification at the beginning of the sketch. Since I modified it for my purposes, then yes, I agree this version is now mine. If anything, it seems YOU are the one with the attitude, bud.

if there is anyone else willing to help me understand my code in a constructive way, i would greatly appreciate it.

I think that the comments are directed towards your apparent sloppy thinking, How would anyone here understand your issues when you fail to keep your code and comments current with the Sketch AS IT IS NOW. WE AREN'T MIND READERS EVEN YOUR ALLEGED MIND. YOUR COMMENTS ARE INAPPROPRIATE TO THE CODE AND MISLEADING TO THOSE TRYING TO HELP. IF YOU WISH BETTER TREATMENT DON'T EXPECT ANYONE TO TRY TO FATHOM YOUR INTENTIONS FROM SLOPPY CODE, DELIBERATELY SO BEFORE SUBMISSION AS YOU CERTAINLY KNOW MORE ABOUT WHAT IS REAL AND WHAT IS JUNK LEFT FROM PREVIOUS NON WORKING ATTEMPTS TO FIX YOUR ILL PLANNED SKETCH. INSULTING... PERHAPS BUT MUCH DEPENDS ON YOUR EFFORTS BEFORE WE CAN TRY TO SEE YOUR ISSUES AND MISLEADING COMMENTS ARE A MOST POOR WAY TO BEGIN AND FURTHER A WASTE OF MY OR OTHERS TIME... THE PERSON RESPONDING WAS TRYING TO HELP YOU BY POINTING OUT A SOURCE OF FRUSTRATION AND APPARENT MISINFORMATION IN READING YOUR ATTEMPTS, NOT "PICKING ON YOU...

Doc
Oh My dioids I have my caps lock button on... gee I guess everyone will understand that I didn't mean it...
Oh yeah STEALING, I should hope so... That's what "Open Source" is all about...
I STEAL CODE ANYTIME I CAN... BUT only open source... It's a joke...

Man, you guys are really over the top tonight.

@wannabe_pilot,
Udo Klein posts here regularly, try sending him a PM to ask about the code. I don't really follow what it's doing.

thank you crossroads. i've seen that you are a helpful and respected member here. thanks for taking the time to reply.

i have noticed that udo klein is a member here and i have sent him a PM.

I'm not sure why you're accusing me of stealing; that's quite out of line.

Would you have preferred borrowed, acquired, got permission to use? My point was NOT to accuse you of having acquired the code inappropriately. I'm sorry that it came across that way.

You did, however, take the code as yours to do with as you see fit. No one has a problem with you doing that. That's why code is published in the first place.

Where I did have a problem was with the attitude that "I didn't develop this code, so I have no responsibility to understand it". At least that was the impression I got.

PaulS:

I'm not sure why you're accusing me of stealing; that's quite out of line.

Would you have preferred borrowed, acquired, got permission to use? My point was NOT to accuse you of having acquired the code inappropriately. I'm sorry that it came across that way.

You did, however, take the code as yours to do with as you see fit. No one has a problem with you doing that. That's why code is published in the first place.

Where I did have a problem was with the attitude that "I didn't develop this code, so I have no responsibility to understand it". At least that was the impression I got.

no hard feelings. we're all here to learn something, right?

but if i may, the tone of my responses is quite the opposite: "i didn't develop this code, but i want to use it. can someone help me understand it?"

it's a simple question, but even with all the responses, it hasn't been touched on in the slightest...

wannabe_pilot:
if i add too many entries to my array, the program fails to operate correctly.

I notice that you have a spurious trailing comma in the array declaration. I don't know what effect this would have, but if it causes the array to be created with an extra trailing element which is uninitialised then obviously that would cause havoc.

I haven't tried to muddle through the logic inside get_next_count to figure out how it's supposed to work, but as far as I can see it's called with an argument which is the length of the pattern array, and the return value is used as an index into the array. Along the way it's reading and writing EEPROM values.

Is it sensitive to the initialisation state of the EEPROM? I wonder whether you just happen to have written values to the first seven EEPROM locations, or something like that.

I see the return value is modded by the count_limit so should be a valid index into the array, but have you tried printing out the array index before you use it to make sure?

If you find the array index being used and replace the call to get_next_count() with a hard-coded value, do you still get the problem?

Sorry for not answering earlier. I was away on vacation.

@Pauls: the uint8_t vs. uint16_t issue is a bug. I am to blame for it. However it has no effect on the code. With regard to the comments. This is always hard to do. However in my blog I extensively describe how this is supposed to work. Anyway I tested it thouroughly. The bug does not affect the external behaviour. It just does not use the EEPROM in an optimal way once n>255.

He did NOT STEEL my code. What he did is intended use of my code. He only messed slightly by copying the copyright which is of course not correct because it is his code. I am not offended by this. Copyright is tricky.

With regard to the sloppy code: what exactly is sloppy about this code?

Now with regard to the original question: I uploaded the code into an Atmega 328 and 168 and it worked fine with both. I assume the issue is due to the Attiny44. It has only 256 Bytes SRAM and 4k flash. Since the compiled sketch takes slightly above 2k the questions are: are you using a bootloader and if so, how much flash does it consume? Otherwise I suspect you are running out of SRAM. In this case you either have to either learn about optimizing RAM consumption or use a CPU with more RAM. I would vote for more RAM.

Besides that I think you fully understood how to apply my work. Great :slight_smile: Keep going.

FWIW, I originally wrote the program with an UNO board but then trimmed it down for use on an ATTiny44 at 8MHz

Is the problem also seen on the UNO as well as the ATTiny44? Sounds like it may be a memory issue (which is often the case in scenarios where x number of y types works, but when I add one more, it doesn't work, and it doesn't matter which one is added). The ATTiny44 has far less SRAM/EEPROM/Flash memory than the UNO.

He did NOT STEEL my code. What he did is intended use of my code.

I wasn't intending to convey the impression that he acquired the code in any illegal way. I know that you intended for people to take and use the code.

The only point that I was trying to make is that by doing so, he assumed some responsibility for understanding it before trying to modify it.

With regard to the sloppy code: what exactly is sloppy about this code?

I'm not sure who you are addressing this to. I don't recall saying that the code was sloppy. I only mentioned that I didn't see a relationship between some of the names of variables and the purpose that the variables seem to be used for.

Now, I know that naming variables so that for the life of the program, everyone understands the variable to mean the same thing is tricky. It just seems that, in a few cases, either two different variables should have been used, or some better names could have been used.