Difficulty with a Function

[robinfawell]

I would appreciate some help with a function.  I have been successful with some simple ones.

Here is the code, provded by Thorsten, that I want to convert into a function.  There are multiple cases only 2 are shown.  The lengthy code works. When I replace it with the function it fails.

switch (group)

////Memory Group//////uses pin 0 - 5

	 {case(1) :
	{ 
	memory_group_selected_stored_pin = stored_pin

	if (pin_value == 0)// reacts only when pressed, momentary switch action
	//SetLed_NoDump ( 0, 5, 9, (stored_pin));///******* see A below

	   	
	//if button already selected, turn off LED's	
	if(memory_group_selected_stored_pin == pin)//Is current button same as stored pin? ?
	 {
	memory_group_selected_stored_pin = 9;// if same, set to pin 9.  This is the cancel pin
	}else	
	memory_group_selected_stored_pin = pin ;//save the current pin in memory_group_selected_stored_pin variable

	for(i=0; i<= 5; i++)//All LED's  should be cleared	
 	MIOS_DOUT_PinSet0(i); 	
	if(memory_group_selected_stored_pin != 9)
	MIOS_DOUT_PinSet1(memory_group_selected_stored_pin);//set the LED for the current pin
	}

	break;	

//////////Record (Used with Memory Group) /////// uses pin 6

	 {case (2) :{

	record_button_state = stored_pin

	if (pin_value == 0)
	//SetLed_NoDump ( 6, 6, 7, (stored_pin));///!!!!!!! see B below

	if(record_button_state == pin)
 etc

A) The 4th line, that is commented out, is the function sending statement.  B) This is a similar line to A) in Case (2) with the change of the last parameter. Here is my attempt at the function.

void SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, 
unsigned char stored_pin,  unsigned char pin)

	{
	unsigned char i;

	if(pin == (stored_pin) )	
	(stored_pin) == canc_pin;
	else
	(stored_pin) == pin;	
	for (i=low_pin; i<= high_pin; i++)
	MIOS_DOUT_PinSet0(i);
	if (( stored_pin) != canc_pin)
	MIOS_DOUT_PinSet1 ((stored_pin));	
	return;
	}

I am aware that there may be a local/gloal variable problem.  I have tried out many changes but none work!

Thanks in advance.

Robin

[moogah]

Can you provide some more details about what this code is supposed to do and what (if anything) the function actually ends up doing?  I'm pretty good with C, but I've not done any work with the C applications around here yet.  Error messages from the compiler?

if(pin == (stored_pin) )	

why do you have stored_pin in parantheses?  It prolly won't affect the program at all, but this shouldn't be needed.

[moogah]

Ahh.  I'm rusty ;D

I see some problems now:

"==" is the equality operator, use that to check wheather two variables are the same:

if(x==y)

"=" is the assignment operator, use this to assign a value to a variable:

x=1

Or:

x=y

Those parantheses are not needed either, try this:

void SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, 
unsigned char stored_pin,  unsigned char pin)
{
	unsigned int i;

	if(pin == stored_pin )	
		stored_pin = canc_pin;
	else
		stored_pin = pin;	
		for (i=low_pin; i<= high_pin; i++)
			MIOS_DOUT_PinSet0(i);
			if (stored_pin != canc_pin)
			     MIOS_DOUT_PinSet1 (stored_pin);	
		return;
}

Without getting out pen and paper to check your logic seems to be correct.  One last thing:  you are using "i" as a counter variable (i for integer), but you are declaring it as a character.  While this may still work, you should use the type "int" here.  Since you know you don't need to count negative numbers the "unsigned int" is slightly more efficient.

[Thomas]

While this may still work, you should use the type "int" here.  Since you know you don't need to count negative numbers the "unsigned int" is slightly more efficient.

No he shouldn't. He should use "unsigned char" as loang as he don't need neagtive numbers (then he should use "(signed) char")  or values > 255 (>127, <128).

to use a 16bit with SDCC and PIC is a very bad idea! This could increase the code to more than doubled size! (the pic is only 8bit and has only 8bit registers)

[Thomas]

SetLed_NoDump ( 0, 5, 9, (stored_pin));///******* see A below

SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, unsigned char stored_pin,  unsigned char pin)

You tried to call SetLed_NoDump with 4 parameter but it has 5.

->There must be something wrong.

Where does stored_pin come from?

(stored_pin) == pin;

inside SetLed_NoDump does not change stored_pin outside!

stored_pin in SetLed_NoDump is a completey different variable!

Use a globel variable for stored_pin or call it via reference or return it

unsigned char SetLed_NoDump(...){ ...

  return stored_pin;

}

stored_pin = SetLed_NoDump(...);

[moogah]

No he shouldn't. He should use "unsigned char" as loang as he don't need neagtive numbers (then he should use "(signed) char")  or values > 255 (>127, <128).

Yes, that is definatly correct :-[  Java has ruined me ;)

I'm still not seeing why were keeping stored_pin in parantheses... reason?

[robinfawell]

Thanks to both of you for your help.

Thomas,  I found the error with the 5th parameter.  This should have been removed.  It shows as a compiler error.

I have (stored_pin) declared just under VoidNotifyDin Toggle.  Hence it is outside the function.

Moogah, you were right about == versus =.

The application is setting a "Radio" group of momentary contact pin/led. Only one led should light at a time.  The second press of the same button should cause the led to be off. 

System works except for one anomaly.  More later.

Thanks Robin

[robinfawell]

To Moogah & Thomas

There is no need for () on stored_pin.  However there is in

MIOS_DOUT_PinSet1 (stored_pin).  I tried without and get a syntax error.

Led behaviour

I am looking for an explanation why there is a difference in the behaviour of Din pin 0 in the code you helped me with.

Just after reboot, pin needs two presses before coming On.  Other pins change fom Off to On on the first press.

By the way this behaviour exists in the pre "Function" code.

Regards Robin

PS I should have informed you that my first code in the beginning of this post,Case 1 is preceded by

if (pin >= 0 && pin <= 5)

group = 1;

[moogah]

MIOS_DOUT_PinSet1(stored_pin)

In this case you are passing the function "MIOS_DOUT_PinSet1" the variable "stored_pin" which you symbolize by putting the variable in parantheses right after the function:

MIOS_DOUT_PinSet1(stored_pin)

notice that there is no space between the function name and the variable in parantheses.  Unless there is some other detail with the SDCC this is the correct way.

As for the issue with needed two presses for a button to begin working correctly, consider this:

I'll need to take a closer look. 

[robinfawell]

Since sending this post I have done some further reading and realise what Moogah was referring to.  I need to prototype my function for unsigned char and write the function as an unsigned char.  ie No Void.  I'm not there yet.

The function will change the variables.  I need to return values.

I forgot to include the variable pin in the function.  I'm getting there!

I made an error in thinking that my problem was over.  I have spent a few hours trying to find the error but no luck!  There are no Led's on and nothing works in case1 and 2!  The rest of the code is pre Led function and works well.

I cannot find any faults in my logic but its wrong.  The pre function version works fine.

Here are the relevant parts of the code expanded  for completeness. Sorry about the length. I will try to underline the relevant lines.

#include "cmios.h"

#include "pic18f452.h"

void SendNote(unsigned char chn, unsigned char note, unsigned int velocity);

void SendTremelo();

void SendDump(unsigned char dump_index);

void SetLed_NoDump (unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, unsigned char stored_pin);

void RecordSetup ();

//unsigned char toggle_state [7]; // used for single (non "Radio" Group) Led Toggling

unsigned char dump_index; //triggers sysex messages{

/////////////////////////////////////////////////////////////////////////////

// This function is called by MIOS when an button has been toggled

// pin_value is 1 when button released, and 0 when button pressed

/////////////////////////////////u////////////////////////////////////////////

unsigned char memory_group_selected_stored_pin;//Memory Group Pins 0-5

unsigned char record_button_state;//Memory Group Pins 6, Record

unsigned char pedal_group_stored_pin;//Pedal group, Pins 19-25

unsigned char tremelo_strength_button_state;//Tremelo Fort, Pin 28

unsigned char swell_princ_group_stored_pin;//Sw_P Group, Pins 33-34

unsigned char swell_mixt_group_stored_pin;//Sw_M Group, Pins 36-38

unsigned char swell_reed_group_stored_pin;//Sw_R Group, Pin 40

unsigned char great_princ_group_stored_pin;//Gt_P Group, Pins 42-47

unsigned char great_mixt_group_stored_pin;//Gt_M Group, Pin 49

unsigned char great_reed_group_stored_pin;//Gt_R Group, Pins 51-57

unsigned char pedal_couplers_group_stored_pin;// Pedal couplers pins 16 & 17

unsigned char sub_oct_coupler_button_state;//Pedal Sub-Octave pin 18

unsigned char swell_tremelo_select_button_state;//Tremelo Swell Select

unsigned char great_swell_coupler_button_state;//Gt-Sw Coupler Select.

unsigned char great_tremelo_select_button_state;//Tremelo Great Select

//Use PB switch to input on Core 2, however use LED on Core 1

unsigned char i;// used for  MIOS_DOUT_PinSet0

unsigned char group;

unsigned char pin;

unsigned char stored_pin;

void DIN_NotifyToggle(unsigned char pin, unsigned char pin_value) __wparam

{ 

if (pin >= 0 && pin <= 5)

group = 1;

if(pin == 6)

group = 2;

etc;etc

////////Memory Group////

switch (group)

{case(1) :{

if (pin_value == 0)// reacts only when pressed, momentary switch action

SetLed_NoDump (0, 5, 9, stored_pin);// function

memory_group_selected_stored_pin = stored_pin;//sets the variable for this group to the current stored pin after the function above.

}

break;

//////////Record (Used with Memory Group) ///////

case (2) :{

if (pin_value == 0)

//stored_pin = record_button_state;

SetLed_NoDump ( 6, 6, 7, (record_button_state));

}

break;

//////////////////////////////////////////////////////////////////////////////////////

//This function is called to set the Leds for Single Buttons and Group Buttons/////////////////////////////////////////////

/////////////////////////////////////////////////////////////////////////////////////

void SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, unsigned char stored_pin )

char j;

if(pin == stored_pin ) //Is the current pin the same number as he stored pin?

stored_pin = canc_pin;//If so, then alter the stored pin to the cancel pin allocated number for the group

else

stored_pin = pin; //otherwise make the stored pin vale, the current pin value.

for (j=low_pin; j<= high_pin; j++)// using j variable from 0 to 5

MIOS_DOUT_PinSet0(j);//set all pins in the group to "O"

if (stored_pin != canc_pin)//provided the stored value of the pin is not equal to the cancel number

MIOS_DOUT_PinSet1 ( stored_pin);//set the current  stored pin value to a"1

}

I will have to stop and go to bed, my head is spinning.

Regards Robin

[stryd_one]

my head is spinning.

No doubt, just scrolling through that post makes me wish I never logged on tonight  ;)

[robinfawell]

There are several reasons why I had difficulty.

I have in the past constructed simple functions where the use of a function can avoid repetition.  One example is the sending of Midi messages.

The calling function could be

SendNote(0xb0, 0x7b, 0x00);// The midi message B0 7B 00 will be sent.

The related receiving function is

void SendNote(unsigned char chn, unsigned char note, unsigned int velocity) //__wparam

{

MIOS_MIDI_TxBufferPut(chn); 

  MIOS_MIDI_TxBufferPut(note);   

  MIOS_MIDI_TxBufferPut(velocity); 

}

The above type passes values to the function but receives no parameters back.

Function where values are passed back to the calling function

Prototype

In my "difficult" case I needed to send back a value to the calling function.  This involves a different prototype statement at the start of the program

#include "cmios.h"

#include "pic18f452.h"

void SendNote(unsigned char chn, unsigned char note, unsigned int velocity);

void SendTremelo();

void SendDump(unsigned char dump_index);

unsigned char SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, unsigned char stored_pin, unsigned char pin, unsigned char pin_value );

void RecordSetup ();

unsigned char dump_index; //triggers sysex messages

 

Note the 3rd, 4th and 5th lines. These refer to the functions where no values are returned.

Note the 6th line.  This is the prototype statement where the function is preceded by the data type of the variable being returned.  The function title follows. 

Then the parameters or variables are listed (6 are shown).

Calling function

I have shown two cases out of about 12 .  You will see the lines that include SetLedNoDump.  These lines are the important ones; they send the values to the function.Note tha they are preceded by stored_pin. This is the variable that the function returns values to.

switch (group)

{case(1) :

{

stored_pin = memory_group_selected_stored_pin;// change value of stored pin to the group stored value (in this case the memory group.

stored_pin = SetLed_NoDump(0, 5, 9,stored_pin, pin, pin_value); // for the function, set pin_low to 0, pin_high to 5, canc_pin to 9

memory_group_selected_stored_pin = stored_pin;//resets the variable for this group to the current stored pin after the function above.

}

break;

//////////Record (Used with Memory Group) ///////

case (2) :

{

stored_pin = record_button_state;

stored_pin = SetLed_NoDump( 6, 6, 7,stored_pin, pin, pin_value);

record_button_state = stored_pin;

}

break;

Note that in Case 2, the variable to be manipulated is memory_group_selected_stored_pin.

Receiving Function

unsigned char SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, unsigned char stored_pin, unsigned char pin, unsigned char pin_value )

{

if (pin_value == 0)// only react when button is pressed.

if(stored_pin == pin) //Is the current pin the same number as the stored pin?

stored_pin = canc_pin;//If so, then alter the stored pin to the cancel pin allocated number for the group

else

stored_pin = pin; //otherwise make the stored pin vale, the current pin value.

for (i=low_pin; i<=high_pin; i++)// using i variable from 0 to 5

MIOS_DOUT_PinSet0(i);//set all pins in the group to "O"

if (stored_pin != canc_pin)//provided the stored value of the pin is not equal to the cancel number

MIOS_DOUT_PinSet1 (stored_pin);//set the current  stored pin value to a "1"

return (stored_pin);

}

Note the Return statement.

This shows the logic associated with a group of momentary action push buttons with integral leds in a radio group.  The rules are as follows.

Only one lamp may be on.

Pressing a button twice switches it off. Then all lamps will be off.

The causes of my troubles were:-

Ignorance of dealing with Functions requiring return values.

I also forgot to include the leading statement

if (pin_value == 0)// only react when button is pressed.

This is necessary when using momentary action switches.

That's all folks!

Robin

[moogah]

Hey, I'm too busy with work and school this week to have taken a serious look into what your doing to offer insight, but, it looks like you've got the right idea and know how to find the relavant documents to help you along, keep studying and keep trying (and erroring) new things and I'm sure you'll do fine ;D

[audiocommander]

Hi Robin,

The causes of my troubles were: (...)

so; you solved your troubles with that function?

I noticed you are making no use of curly brackets within if-statements. As this is no error, your function will work fine without, but it may increase readability of the code, if you'd use some brackets and indentation; the logic of the blocks gets a lot clearer...

Moreover I found a problem with your for()-loop. As there are also no curly brackets set, I think this might not work as inteded? See the nested version with curly brackets set here; this is probably how the compiler interprets your code:

[tt]

unsigned char SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, unsigned char stored_pin, unsigned char pin, unsigned char pin_value )

{

if (pin_value == 0) { // only react when button is pressed.

//Is the current pin the same number as the stored pin?

if(stored_pin == pin) {  

// alter the stored pin to the cancel pin allocated number for the group

stored_pin = canc_pin;

} else { // if stored_pin IS NOT the current pin

// make the stored pin vale, the current pin value

stored_pin = pin;

// using i variable from 0 to 5

for(i=low_pin; i<=high_pin; i++) {

//set all pins in the group to "O"

MIOS_DOUT_PinSet0(i);

if(stored_pin != canc_pin) {

//provided the stored value of the pin is not equal to the cancel number

//set the current  stored pin value to a "1"

MIOS_DOUT_PinSet1 (stored_pin); 

return (stored_pin);

}

}

}

  }

}

[/tt]

If my interpretation of the logic flow is right, there is a problem with the return value, because there are cases where nothing is returned!

-- so in contrast, I think you want to write this, which would always return the stored_pin, and would so be a correct implementation:

(as you can see, it differs only in indentation and proper setting of curly backets):

[tt]

unsigned char SetLed_NoDump(unsigned char low_pin, unsigned char high_pin, unsigned char canc_pin, unsigned char stored_pin, unsigned char pin, unsigned char pin_value )

{

if (pin_value == 0) { // only react when button is pressed.

//Is the current pin the same number as the stored pin?

if(stored_pin == pin) {  

// alter the stored pin to the cancel pin allocated number for the group

stored_pin = canc_pin;

} else { // if stored_pin IS NOT the current pin

// make the stored pin vale, the current pin value

stored_pin = pin;

// using i variable from 0 to 5

for(i=low_pin; i<=high_pin; i++) {

//set all pins in the group to "O"

MIOS_DOUT_PinSet0(i);

}

}

if(stored_pin != canc_pin) {

//provided the stored value of the pin is not equal to the cancel number

//set the current  stored pin value to a "1"

MIOS_DOUT_PinSet1 (stored_pin); 

}

}

return (stored_pin);

}

[/tt]

Cheers,

Michael

[robinfawell]

Thanks Michael

I have revised the code as you suggest.  I agree that it is good practice to always use brackets.  I have avoided them when there is only one statement following that is relevant.

As far as the logic is concerned I agree about the return statement.  I suppose that I was lucky.  In my case the results are the same. However the compiler, I think, will warn you if the return value is not sent

However I will indent the lines and insert brackets in future. 

Thanks for your interest.

Robin