Fotobox - guidance and feedback please

Hello, I have made myself a “fotobox” a device to help me control my camera in high speed photo.
It has 4 buttons and a lcd and outputs are done through a optocoupler.

I have read all i know about programming myself and the code will maybe show that things have progressed, I hope someone will be able to give me some advice as to thing i do wrong or maybe if things could be done in a different way.

The program works, but I’m interested in getting a second oppinion, and any feedback is welcome.

Reg. Hans :slight_smile:

tried posting the code but was too big :frowning: - attached .ino file

FotoBox2013.ino (8.73 KB)

Ok, i know im not perfect here so ,,, any comments ? anything at all ?

void get_buttons(){

Why is a "get" function "void"?
Surely it should return a value?

if(button_plus) drops++;drops = constrain(drops,1,10);button_plus = false;

Don't like the format - regardless of whether "button_plus" is true, you set it to false - is that intentional?

if(button_plus) 
{
  drops++;
  drops = constrain(drops,1,10);
  button_plus = false;
}

would better, wouldn't it?

Does it work and do what you want? If so, what do you care what I say?

I find the style a bit wonky.

	if(button_minus)	analog_sensor = false;  button_minus = false;
if(button_right) menu = menuline[mode][++menuitem];button_right=false;
if(button_left) menu = menuline[mode][--menuitem];button_left=false;

The layout gives the impression that there are multiple statements in the if, but the one on the right is not within the if. It would be more conventional to write:

	if(button_minus)
            analog_sensor = false;
        button_minus = false;

	if(button_right)
            menu = menuline[mode][++menuitem];
        button_right=false;

	if(button_left)
            menu = menuline[mode][--menuitem];
        button_left=false;

But to each his own.

You used sprintf() in several places where strcpy() would be fine. But you use sprintf() with %c and %i a few times, so you wouldn't be able to eliminate it entirely.

You use several strings that I would put in PROGMEM, -- eg:

sprintf(lcdline1,"SENSOR SETUP") ==> strcpy_P(lcdline1, PSTR("SENSOR SETUP"))

But I am super-paranoid about wasting RAM. Maybe more than you.

On line 170 you have nr and delay_inc passed to sprintf() with no corresponding items in the pattern.

	sprintf(lcdline1,"DELAY TIME  ",nr,delay_inc);
	if(button_right) menu = menuline[mode][++menuitem];button_right=false;
if(button_left) menu = menuline[mode][--menuitem];button_left=false;

I don’t see any code to keep menuitem from walking off the beginning or end of the array.

I find this whole construction a bit difficult to follow:

#define flipbit(ADDRESS,BIT) (ADDRESS ^= (1<<BIT))

#define shutter flipbit(PORTB,5) //13

shutter;
delay(100);
shutter;

In my view it would be more readable to do something like

const int SHUTTER = 13;

setup() {
   pinMode(SHUTTER, OUTPUT);
}

shutter_release()
{
    digitalWrite(SHUTTER, 1);
    delay(100);
    digitalWrite(SHUTTER, 0);
}

Quote
Code:
if(button_minus) analog_sensor = false; button_minus = false;
if(button_right) menu = menuline[mode][++menuitem];button_right=false;
if(button_left) menu = menuline[mode][--menuitem];button_left=false;

The layout gives the impression that there are multiple statements in the if, but the one on the right is not within the if.

Okidoki, i see. I have been under the impression that this would have the same "effect" as :

if(button_minus)	analog_sensor = false;  button_minus = false; 
=
if(button_minus) {
	analog_sensor = false;  
        button_minus = false;
}

Guess i had that wrong :slight_smile: thank you :wink:

get_buttons - now called read_buttons :wink: TY

It works , and it does as it is supposed to :slight_smile:
I thank you for your time looking at my code and your comments they are appreciated.
I am still learning this from reading around on the web and as I see here there are some things i have gotten wrong (the if thing on one line that doesent quite do as i thought). And as you point out I would love to know how to shrink memory use and also code space.

Will read up on PROGMEM and see if i can use that more efficiently.

The sprintf thing, i see there are some leftovers from older versions where i did need sprintf, i'll remove those , would it be good then to use the F() function to save mem?