2
\$\begingroup\$

Within our project we've used a lot of boolean setOutputValue(char pinNumber, boolean pinValue) kind of functions.

Within those functions we do the following (for example):

hardware_layer:

#define L9823_HIGH  0
#define L9823_LOW   1
void hardware_L9823_setOutput(U8BIT pin, U8BIT value)
{
  /* some SPI interfacing is handled here */
}

IO_layer:

#define IO_ACTIVE   1
#define IO_INACTIVE 0
void io_outputDriver_setOutput(U8BIT pin, U8BIT value)
{
  if (value == IO_ACTIVE)
  {
    hardware_L9823_setOutput(pinMapping[pin], L9823_HIGH);
  }
  else
  {
    hardware_L9823_setOutput(pinMapping[pin], L9823_LOW);
  }
}

component_layer:

#define COMPONENT_ASIMPLEDEVICE_A_ON    1
#define COMPONENT_ASIMPLEDEVICE_A_OFF   0
void component_aSimpleDeviceA_set(U8BIT value)
{
  if (value == COMPONENT_ASIMPLEDEVICE_A_ON)
  {
    io_outputDriver_setOutput(DEVICE_PIN_A, IO_INACTIVE);
  }
  else
  {
    io_outputDriver_setOutput(DEVICE_PIN_A, IO_ACTIVE);
  }
}

#define COMPONENT_ASIMPLEDEVICE_B_ON    1
#define COMPONENT_ASIMPLEDEVICE_B_OFF   0
void component_aSimpleDeviceB_set(U8BIT value)
{
  if (value == COMPONENT_ASIMPLEDEVICE_B_ON)
  {
    io_outputDriver_setOutput(DEVICE_PIN_B, IO_INACTIVE);
  }
  else
  {
    io_outputDriver_setOutput(DEVICE_PIN_B, IO_ACTIVE);
  }
}

unit_layer:

#define COMPLEXDEVICE_ON    1
#define COMPLEXDEVICE_OFF   0
void component_aComplexDeviceWithMultipleComponents_set(U8BIT value)
{
  if (value == COMPLEXDEVICE_ON)
  {
    component_aSimpleDeviceA_set(COMPONENT_ASIMPLEDEVICE_A_ON);
    component_aSimpleDeviceB_set(COMPONENT_ASIMPLEDEVICE_A_ON);
  }
  else
  {
    component_aSimpleDeviceA_set(COMPONENT_ASIMPLEDEVICE_A_OFF);
    component_aSimpleDeviceB_set(COMPONENT_ASIMPLEDEVICE_A_OFF);
  }
}

This is a rather simple example, but in our case we have a total of 4 layers to seperate hardware/io/component and module implementations. So this has to propagate 4 layers down before it reaches the hardware register.

It does give less code to maintain and less comments to maintain (each function has a header of atleast 9 lines of code.

I've raised the concern of having many if-statements while trying to set a hardware register and proposed to start splitting these functions in to each having their own function like so:

boolean setOutputLow(char pinNumber)
{
    if (pinNumber is within limits of output register mapping)
    {
        regMapping[pinNumber] = OUTPUTLOW;
    }
}

and the same ofc. for the High variant. Which seems to save a lot of if-else logic down the road but does increase code size a tiny bit.

Which would be a prefered way of handling these kind of things? Are there even better ways? Or is this the right road to walk?

\$\endgroup\$
  • \$\begingroup\$ void aModuleSetValue(boolean lowHigh) { setOutputValue(PINBELONGINGTOTHISMODULE, lowHigh); } would be a starting point. \$\endgroup\$ – Morwenn Apr 22 '13 at 14:01
  • \$\begingroup\$ @Morwenn Sadly that is not allowed according to the architecture :-( Say layer 1 = lowest and layer 5 = highest layer. We are never allowed to include 'upwards' and downwards we are only allowed to include the layer directly below the current one. \$\endgroup\$ – Daan Timmer Apr 22 '13 at 14:17
  • \$\begingroup\$ Er, what I did has IMHO nothing to do with architecture: I just proposed to change if (a==false) b(false); else b(true); by b(a). It is just a static analysis optimization. It seems that you are talking as if I tried to modify setOutputValue in your previous comment :o \$\endgroup\$ – Morwenn Apr 22 '13 at 14:24
  • \$\begingroup\$ I understand what you ment. The values to 'set' to the lower levels just need to be seperated from the higher level functions. That means the higher level functions don't know what value to set except for the value below their own layer. \$\endgroup\$ – Daan Timmer Apr 22 '13 at 14:26
4
\$\begingroup\$

This would be better if you could post a real extract of the four layers and the real proposed C replacement code rather than pseudocode.

But you might also consider why software four levels up from the hardware even knows about the names of registers and the polarity of signals at pin level. In other words, the upper level might know that it wants to turn on the "bit-digester" (or some such), but only the driver (ie. lower) layer needs to know which register/bit/level that involves. That is just my general feeling without knowing any details of your project - of course there might be good reasons for this information to propagate upwards in your application, but it needs to be strong reason to propagate very far...


EDIT after OP updated question

There are always many ways to do these things and my guess is that someone senior set down this arrangement and will be unimpressed by you suggesting it should be changed.

However, that doesn't mean you shouldn't try :-) I can think of a number of approaches:

  1. What you currently have. This has the advantage of being simple to understand, but is verbose and involves many conditionals.

  2. What others have suggested, namely passing the 'value' straight through without the conditionals. This would be more efficient but effectively extends a dependency from the hardware level all the way up through these layers. The dependency is hidden because the values passed are just assumed to be all the same - 1 on, 0 off. But it is a dependency all the same and so I don't subscribe to this option.

  3. Use a translation at each level that doesn't involve a conditional. For example in component_deviceA_set

    #define COMPONENT_DEVICE_A_MASK    0x1
    void component_deviceA_set(U8BIT value)
    {
        static const U8BIT level[] = {IO_INACTIVE, IO_ACTIVE};
        const U8BIT output = level[value & COMPONENT_DEVICE_A_MASK];
        io_outputDriver_setOutput(DEVICE_PIN_A, output);
    }
    

    When I try this, I get significantly less code and no conditional as long as the compiler cannot inline the call to io_outputDriver_setOutput. If the compiler can inline that function, it recreates the conditional (looking in the assembler on x86).

    So this approach is probably faster than what you have, and the level[] array need not be repeated for every such function. But on the downside it involves an extra #define and becomes more complicated if there are cases where there are, say, 3 options instead of 2 (ie. what to do about the invalid 4th option covered by a mask of 0x11).

  4. Use the functional approach you suggest. This leads to even simpler code than (3) and in my mind gives the nicest result. It does lead to a doubling of the number of functions but they are all trivial and take no parameters. The noisy #defines (COMPONENT_DEVICE_B_ON, etc) disappear, which is nice.

So (4) looks like the nicest option to me.

Note that if the compiler can see the guts of all of these functions it might be able to optimise away the conditional in option (1). But I expect they are in different files, so that will not happen.

Note also that using U8BIT instead of int gains nothing and in fact probably makes the code slightly slower, because the compiler will insert byte-to-long sign extensions when passing them around. These U8BIT parameters are also dishonest, because the value parameters don't contain a 'bit', they contain a logical on/off state (or conceivably an on/off/standby etc); yes that in general does come down to a bit at the lower level, but the upper level has no business knowing that. As far as the upper level is concerned it is just turning something on and whether that involves setting one or a hundred bits is not its concern.

Note also that these U8BIT parameters cause warnings in my compiler ("passing argument 1 of 'io_outputDriver_setOutput' with different width due to prototype") due to my default warning settings

\$\endgroup\$
  • \$\begingroup\$ Updated my question with real C instead of the pseudocode \$\endgroup\$ – Daan Timmer Apr 23 '13 at 7:23
  • \$\begingroup\$ Also I think either I said it wrong or you read it wrong. But we only propagate down and never up. We are only allowed to call one layer down. We are never allowed to call upwards. (where down = hardware and up = unit) \$\endgroup\$ – Daan Timmer Apr 23 '13 at 8:03
  • \$\begingroup\$ We are using a microcontroller so the x86 compiler might indeed be complaining. It is a 16-bit fujitsu (ugh) processor and using the softune provided toolchain. Your (3) solution indeed looks nice. And if we didnt have to do boundary checking then I surely would have picked that. Sadly boundary check adds an extra conditional :-) \$\endgroup\$ – Daan Timmer Apr 23 '13 at 14:30
  • \$\begingroup\$ I've discussed your answer with our team and we all agreed upon starting to use the (4) approach. Thanks for backing it up. \$\endgroup\$ – Daan Timmer Apr 26 '13 at 9:45
  • \$\begingroup\$ That is good news :-) \$\endgroup\$ – William Morris Apr 26 '13 at 11:54
3
\$\begingroup\$

I am really confused why the following code exists in your upper level.

void aModuleSetValue(boolean lowHigh)
{
    if (lowHigh == false)
    {
        setOutputValue(PINBELONGINGTOTHISMODULE, false);
    else
    {
        setOutputValue(PINBELONGINGTOTHISMODULE, true);  //BTW, corrected spelling
    }
}

If that variable lowHigh is truly boolean, better code would be to simply pass it on to the lower level for the PIN associated with the method.

void aModuleSetValue(boolean lowHigh)
{
    setOutputValue(PINBELONGINGTOTHISMODULE, lowHigh);
}

If lowHigh is (somehow) something more "truthy" than strictly the boolean values true or false (0 or 1 when cast to an integer, IIRC), you could also convert appropriately. This might be redundant, but I don't remember what C requires to happen if a "truthy" int (either a value not zero or zero) needs to be cast to a boolean type instance other than that only zero is regarded as false when in a boolean expression.

void aModuleSetValue(int lowHighTruthy)
{
    //explicitly restrict a "truthy" integer to one of only two values.
    boolean lowHigh = lowHighTruthy? true: false;  
    setOutputValue(PINBELONGINGTOTHISMODULE, lowHigh);
}

Tell me about that, first, and then we can consider whether the other blocks are needed.

\$\endgroup\$
  • 1
    \$\begingroup\$ Exactly what I was trying to say :D \$\endgroup\$ – Morwenn Apr 22 '13 at 17:42
  • 1
    \$\begingroup\$ @JayC I have updated my question with a more thorough example of how the software is right now. We are not allowed to have implicit boolean logic because we have to comply with MISRA-C. Thus A ? B : C is not allowed, A == VALUE ? B : C is though. \$\endgroup\$ – Daan Timmer Apr 23 '13 at 7:23
0
\$\begingroup\$

The lowest layer should be something like an "SPI controller driver", whose job is to abstract the details of the controller's hardware from higher levels. It might provide functions like send_bit(device, bit), bit = get_bit(device), bit = exchange_bits(device, bit) and the same for sending/receiving bytes instead of bits.

At next layer should be "SPI device drivers". These rely on the abstraction provided by the "SPI controller driver" to talk to a device without caring about any of the controller's details. An "SPI device driver" would also provide some sort of abstraction to higher levels; but the nature and design of this abstraction depends on the type of device. For example, it might be a read_temperature() function and nothing else, or it could be a play_sound() function and nothing else, or it might be a pair of functions (e.g. read() and write()) to transfer data to an SD card, or whatever else makes sense for the specific type of device.

There is no need for other layers; and having 4 different layers is likely to cause unnecessary confusion for no benefit. A good example of this is your own io_outputDriver_setOutput() function.

\$\endgroup\$
  • \$\begingroup\$ Sadly that is something that I can't change any more. I agree with you. But this is how the hierarchy was decided upon a year or two ago. Including what type of 'modules' go in what layer. Yet I still wholeheartily agree with you. And if I could change that part I definitely would. \$\endgroup\$ – Daan Timmer Apr 23 '13 at 8:43

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.