How to replace this switch with preprocessor code.... whats theadvantage??

[Rohit]

Hi,

I am working on a switch module which after reading voltage through a
port pin and caterogizing it into three ranges(open,low or high),
passes this range to a function switch_status() with parameters value
and signal ID. Signal Id is used to get a user configurable parameter
inside a configuration file, which depends on the type of switch.

I have implemented it as under. Please ignore those magic numbers as I
have mimized logic to simplify it. Now I want to optimize it. Somebody
told me that the logical connection between variables: value,
switchconfTable[signal_id].contact and the return value(TRUE or FALSE)
should be solved via preprocessor not code.

I am unclear about what it means. Can anybody suggest an insight what
it means ?


..h file

typedef enum
{
Switch_Low, /*!< 0 - value of digital Input is low */
Switch_High, /*!< 1 - value of digital Input is high */
Switch_Open /*!< 2 - value of digital input being not
connected (break) */
} Switch_value_t;

..c file
/* This function is called after determining that value
is in one of the ranges defined by Switch_value_t */
int static switch_status(Switch_value_t value, signal_id)
{
int retval = 0;
switch(value)
{
case Switch_Low:
{
if(switchconfTable[signal_id].contact==0x01)
retval = 1;

else
retval = 2;

}

case Switch_High:
/* Similar code as Switch_Low case*/

case Switch_Open:
/* Similar code as Switch_Low case*/
}

return retval;
}

Cheers
Rohit

 

[Thad Smith]

Hi,

I am working on a switch module which after reading voltage through a
port pin and caterogizing it into three ranges(open,low or high),
passes this range to a function switch_status() with parameters value
and signal ID. Signal Id is used to get a user configurable parameter
inside a configuration file, which depends on the type of switch.

I have implemented it as under. Please ignore those magic numbers as I
have mimized logic to simplify it. Now I want to optimize it. Somebody
told me that the logical connection between variables: value,
switchconfTable[signal_id].contact and the return value(TRUE or FALSE)
should be solved via preprocessor not code.

I am unclear about what it means. Can anybody suggest an insight what
it means ?

I don't know what the comment means or how it might apply to your code. I
see nothing in the code that would be better with the use of macros and I
use more macros than most programmers.

.h file

typedef enum
{
Switch_Low, /*!< 0 - value of digital Input is low */
Switch_High, /*!< 1 - value of digital Input is high */
Switch_Open /*!< 2 - value of digital input being not
connected (break) */
} Switch_value_t;

.c file
/* This function is called after determining that value
is in one of the ranges defined by Switch_value_t */
int static switch_status(Switch_value_t value, signal_id)

I suggest adding a blank line for visual separation between the comment
line and the function statement.

You omitted the type of signal_id. To avoid this kind of problem in posted
code, cut and paste compiled code.

{
int retval = 0;
switch(value)
{
case Switch_Low:
{
if(switchconfTable[signal_id].contact==0x01)
retval = 1;

else
retval = 2;

}

You probably want a break here before the next case.

 

[Rohit]

Hi,

I am working on a switch module which after reading voltage through a
port pin and caterogizing it into three ranges(open,low or high),
passes this range to a function switch_status() with parameters value
and signal ID. Signal Id is used to get a user configurable parameter
inside a configuration file, which depends on the type of switch.

I have implemented it as under. Please ignore those magic numbers as I
have mimized logic to simplify it. Now I want to optimize it. Somebody
told me that the logical connection between variables: value,
switchconfTable[signal_id].contact and the return value(TRUE or FALSE)
should be solved via preprocessor not code.

I am unclear about what it means. Can anybody suggest an insight what
it means ?

I don't know what the comment means or how it might apply to your code.  I
see nothing in the code that would be better with the use of macros and I
use more macros than most programmers.

.h file

typedef enum
{
    Switch_Low,           /*!<  0 - value of digital Input is low */
    Switch_High,          /*!<  1 - value of digital Input is high */
    Switch_Open          /*!<   2 - value of digital input being not
                                connected (break) */
} Switch_value_t;

.c file
/* This function is called after determining that value
     is in one of the ranges defined by Switch_value_t */
int static switch_status(Switch_value_t value, signal_id)

I suggest adding a blank line for visual separation between the comment
line and the function statement.

You omitted the type of signal_id.  To avoid this kind of problem in posted
code, cut and paste compiled code.

{
  int retval = 0;
  switch(value)
        {
            case Switch_Low:
                  {
                      if(switchconfTable[signal_id].contact==0x01)
                            retval = 1;

                      else
                            retval = 2;

                  }

You probably want a break here before the next case.

            case Switch_High:
                     /* Similar code as Switch_Low case*/

           case Switch_Open:
                     /* Similar code as Switch_Low case*/
        }

    return retval;
}

OK here is the real code:

Content of configuration table SwitchConfigTab can be understood
easily by going through the code. its an array of structures
containing configuration for each switch.

typedef enum
{
Switch_Low, /*!< 0 - value of digital Input is low */
Switch_High, /*!< 1 - value of digital Input is high */
Switch_Open /*!< 2 - value of digital input being not
connected (break) */

} Switch_value_t;


typedef enum
{

Switch_01, /*!< ID for Switches / Push buttons */
Switch_02, /*!< ID for Switches / Push buttons */
Switch_03, /*!< ID for Switches / Push buttons */
Switch_04, /*!< ID for Switches / Push buttons */
Switch_05, /*!< ID for Switches / Push buttons */
Switch_06, /*!< ID for Switches / Push buttons */
Switch_07, /*!< ID for Switches / Push buttons */
} signalID_t;

static void CheckSwitchStatus(Switch_value_t value,
signalID_t
SignalID)
{
/* Check value */

switch (value)
{
/* IF the value is open for enabled switches then,irrespective
of
switch type and its test condition update switch status &
value based
on Switch contact.
*/

case Switch_Open:
{
/* Check Switch contact:
For normally open switches - Switch_open means switch is
in
Neutral position.
For normally closed switches-Switch_open means switch is
in
Active position
*/


if (SwitchConfigTab[SignalID].SwitchContact == Switch_NC
{
SwitchObjTable[SignalID].SwitchCurrentStatus =
Switch_Active;
}
else
{
SwitchObjTable[SignalID].SwitchCurrentStatus
=Switch_Neutral
}

SwitchObjTable[SignalID].ValueState = VALUE_FULLVALID;


break;
}

case Switch_Low:
{
/* For EnableLow type switches-value = low means no error
condition.
Check for switch contact and update the status.
*/

if (SwitchConfigTab[SignalID].SwitchType ==
Switch_EnableLow
{

/* If Noramally closed contact switches update switch
status as
Neutral.For normally open switches update switch status as
Active.
Call UpdateSwitchStatus function to update status.
*/

if (SwitchConfigTab[SignalID].SwitchContact == Switch_NC
{
SwitchObjTable[SignalID].SwitchCurrentStatus =
Switch_Active;
}
else
{
SwitchObjTable[SignalID].SwitchCurrentStatus
=Switch_Neutral
}

SwitchObjTable[SignalID].ValueState = VALUE_FULLVALID;


break;
}

/* For enable high switches value = Low means error is
present
for the switch.*/

else if (SwitchConfigTab[SignalID].SwitchType ==
Switch_EnableHigh)

{
/* In error conditions assign default status to the switch
status
and update value state as error.*/


SwitchObjTable[SignalID].SwitchCurrentStatus =
Switch_DefaultState;

SwitchObjTable[SignalID].ValueState =
VALUE_ERROR;


/* Check if condition is enabled by the user.
If it's enabled,then call the error handler function to
set the
error.*/

if (SwitchConfigTab[SignalID].TestEnable ==
Switch_Test_SwitchToLow )

{

(void)ErrHdl_SetTestFailed(SwitchConfigTab[SignalID].ErrorNumber);

}

break;
}


break;

} /*End case Switch_Low*/

case Switch_High:
{

SIMILIAR CODE AS IN ABOVE CASE;

break;

}/*End case Switch_High*/

default:
break;

}/* End of Switch statement*/

}

Cheers
Rohit

 

[Ben Bacarisse]

OK here is the real code:

It can't be. There are syntax errors. The layout is so unusual that
I, personally, can't follow it. I don't want you to change it -- I am
just explaining why I can't help.

typedef enum
{

Switch_01, /*!< ID for Switches / Push buttons */
Switch_02, /*!< ID for Switches / Push buttons */
Switch_03, /*!< ID for Switches / Push buttons */
Switch_04, /*!< ID for Switches / Push buttons */
Switch_05, /*!< ID for Switches / Push buttons */
Switch_06, /*!< ID for Switches / Push buttons */
Switch_07, /*!< ID for Switches / Push buttons */
} signalID_t;

This is very odd. Does it help to name the numbers from 0 to 6 which
is what this enum does? It might do, but I think is worth pointing
out that such enums are unusual. By the way, I think the comments get
in the way rather help explain this enum.

<snip more code>

 

[August Karlstrom]

OK here is the real code:

Content of configuration table SwitchConfigTab can be understood
easily by going through the code. its an array of structures
containing configuration for each switch.

typedef enum
{
Switch_Low, /*!< 0 - value of digital Input is low */
Switch_High, /*!< 1 - value of digital Input is high */
Switch_Open /*!< 2 - value of digital input being not
connected (break) */

} Switch_value_t;

[...]

After copying your code to the file test.c:

$ gcc -ansi -pedantic -Wall test.c
test.c:21: warning: comma at end of enumerator list
test.c: In function ‘CheckSwitchStatus’:
test.c:50: error: ‘SwitchConfigTab’ undeclared (first use in this function)
test.c:50: error: (Each undeclared identifier is reported only once
test.c:50: error: for each function it appears in.)
test.c:50: error: ‘Switch_NC’ undeclared (first use in this function)
test.c:51: error: expected ‘)’ before ‘{’ token
test.c:65: error: expected expression before ‘}’ token
test.c:75: error: ‘Switch_EnableLow’ undeclared (first use in this function)
test.c:76: error: expected ‘)’ before ‘{’ token
test.c:156: error: expected expression before ‘}’ token
test.c:29: warning: enumeration value ‘Switch_High’ not handled in switch
test.c:158: error: expected declaration or statement at end of input


August

 

[Keith Thompson]

OK here is the real code:

Content of configuration table SwitchConfigTab can be understood
easily by going through the code. its an array of structures
containing configuration for each switch.

typedef enum
{
Switch_Low, /*!< 0 - value of digital Input is low */
Switch_High, /*!< 1 - value of digital Input is high */
Switch_Open /*!< 2 - value of digital input being not
connected (break) */

} Switch_value_t;

If the specific values are significant, I'd declare them explicitly:

typedef enum
{
Switch_Low = 0,
Switch_High = 1,
Switch_Open = 2
} Switch_value_t;

It's exactly equivalent, but it more clearly epxresses the intent.

{

Switch_01, /*!< ID for Switches / Push buttons */
Switch_02, /*!< ID for Switches / Push buttons */
Switch_03, /*!< ID for Switches / Push buttons */
Switch_04, /*!< ID for Switches / Push buttons */
Switch_05, /*!< ID for Switches / Push buttons */
Switch_06, /*!< ID for Switches / Push buttons */
Switch_07, /*!< ID for Switches / Push buttons */
} signalID_t;

Here I assume the specific values *aren't* significant, because they
don't match the names of the constants (Switch_01 == 0, Switch_02 ==
1, etc.). I can imagine this causing problems if you're not careful
-- so be careful. (You might consider adding a "Dummy_00" constant at
the beginning.)

[...]

 

[Rohit]

OK here is the real code:

Content of configuration table SwitchConfigTab can be understood
easily by going through the code. its an array of structures
containing configuration for each switch.

typedef enum
{
    Switch_Low,           /*!<  0 - value of digital Input is low */
    Switch_High,          /*!<  1 - value of digital Input is high */
    Switch_Open          /*!<   2 - value of digital input being not
                                connected (break) */

} Switch_value_t;

[...]

After copying your code to the file test.c:

$ gcc -ansi -pedantic -Wall test.c
test.c:21: warning: comma at end of enumerator list
test.c: In function ‘CheckSwitchStatus’:
test.c:50: error: ‘SwitchConfigTab’ undeclared (first use in this function)
test.c:50: error: (Each undeclared identifier is reported only once
test.c:50: error: for each function it appears in.)
test.c:50: error: ‘Switch_NC’ undeclared (first use in this function)
test.c:51: error: expected ‘)’ before ‘{’ token
test.c:65: error: expected expression before ‘}’ token
test.c:75: error: ‘Switch_EnableLow’ undeclared (first use in this function)
test.c:76: error: expected ‘)’ before ‘{’ token
test.c:156: error: expected expression before ‘}’ token
test.c:29: warning: enumeration value ‘Switch_High’ not handled in switch
test.c:158: error: expected declaration or statement at end of input

August- Hide quoted text -

- Show quoted text -

I was looking for a suggestion as to how can I decouple the connection
between various checks through preprocessor instead of implementing it
in code. And I am sure above code gives enough relevant info to serve
the purpose. I did not give all data structures as they are
irrelevant, only used for comparison and taking a decision.

What you have given above suggests that you do not have any experience
in reading code other than your own. Dont be just a syntax parser try
to think also.

 

[Keith Thompson]

OK here is the real code:

Content of configuration table SwitchConfigTab can be understood
easily by going through the code. its an array of structures
containing configuration for each switch.

typedef enum
{
Switch_Low, /*!< 0 - value of digital Input is low */
Switch_High, /*!< 1 - value of digital Input is high */
Switch_Open /*!< 2 - value of digital input being not
connected (break) */

} Switch_value_t;

[...]

After copying your code to the file test.c:

$ gcc -ansi -pedantic -Wall test.c
test.c:21: warning: comma at end of enumerator list
test.c: In function 'CheckSwitchStatus':
test.c:50: error: 'SwitchConfigTab' undeclared (first use in this function)
test.c:50: error: (Each undeclared identifier is reported only once
test.c:50: error: for each function it appears in.)
test.c:50: error: 'Switch_NC' undeclared (first use in this function)
test.c:51: error: expected ')' before '{' token
test.c:65: error: expected expression before '}' token
test.c:75: error: 'Switch_EnableLow' undeclared (first use in this function)
test.c:76: error: expected ')' before '{' token
test.c:156: error: expected expression before '}' token
test.c:29: warning: enumeration value 'Switch_High' not handled in switch
test.c:158: error: expected declaration or statement at end of input

I was looking for a suggestion as to how can I decouple the connection
between various checks through preprocessor instead of implementing it
in code. And I am sure above code gives enough relevant info to serve
the purpose. I did not give all data structures as they are
irrelevant, only used for comparison and taking a decision.

What you have given above suggests that you do not have any experience
in reading code other than your own. Dont be just a syntax parser try
to think also.

If you write "OK here is the real code:", it's reasonable to assume
that you've posted something that will compile. Too many people post
paraphrases of their actual code that hide the problem they're asking
about. I'm not saying you've done this, but posting an inexact
summary of your actual code just makes it harder for us to see what's
going on.