Trying to condense code while keeping modularity with bitwise operator

Question

I'm working on an embedded systems project, and am reading multiple switches, and then doing things depending on the result. I am trying to keep this modular and abstracted, so each of my functions don't see any of the low-level pin numbers or pin reading functions.

There may be multiple switches down at a time, so I am storing the numbers using bitwise or |, then using bitwise to compare. I am currently just redundantly converting the pin that is read, into my switch values that can be compared with bitwise operators.

Is the a more efficient or better way to do this?

// physical pins on microcontroller
#define pin_sw_green 5
#define pin_sw_yellow 6
#define pin_sw_blue 7
#define pin_sw_red 8


// switch numbers, allowing bitwise operators to work
#define switch_green 0x01 
#define switch_yellow 0x02 
#define switch_blue 0x04 
#define switch_red 0x08 


// store switch press to val
uint8_t button_pressed()
{
  uint8_t data;
  if (pin_read(pin_sw_red))
    data |= switch_red;
  //...
  if (pin_read(pin_sw_green))
    data |= switch_green;    
  return data;
}

 //...

uint8_t button_data = button_pressed();
if (button_data & switch_red)
{
// do things..

I tend to avoid them as it makes the code indeterminate. For this code polling is plenty good. But even if I did use them, I would still have this issue — Kyle Hunter, Sep 18 '18 at 00:39
If you insist. But I do not agree with the sentiment about interrupts though. — bigwillydos, Sep 18 '18 at 01:02
@bigwillydos Maybe he isn't using interrupts, because doing so on a button input pin is a bad idea in general? You _can_ do it, but it requires a lot of extra code and complexity to handle it properly. See [this](https://stackoverflow.com/a/23559522/584518). So tossing out some generic advise "why don't you use interrupts" without mentioning de-bouncing is not helpful. — Lundin, Sep 18 '18 at 07:43
Polling is 1980ies coding practice where embedded systems often did not even have interrupts for most peripherals and hardware debugging support was rudimentary at best. Get used to interrupts and profiling, debugging, etc. It will make things a lot easier when used correctly. Polling is in fact worse, as it makes the whole code highly interdependent. — too honest for this site, Sep 18 '18 at 09:58
@Lundin: I have to disagree. Using interrupts with input pins is just fine when done correctly (it requires usage of a timer(-thread),, too). It does not really increase complexity, but provides more flexibility. — too honest for this site, Sep 18 '18 at 10:01
@toohonestforthissite Did you read the link I posted? That method is about the only way to do it. It's a common beginner bug in embedded systems to just hook up a button to an interrupt pin, which will then in turn completely annihilate real-time performance and possibly the stack as well, because of signal bounces causing 50 interrupts instead of 1. Adding external RC filters is an unnecessary cost, given a competent programmer. — Lundin, Sep 18 '18 at 10:03
@Lundin I well know how to handle mechanical contacts. Also correlating hardware and software is always a good thing. and the complexity depends on the MCU used. OP does not provide this information, hence your atatement "it's much more difficult" is just wrong from a system's view. Note that I wrote "when done correctly" which - of course - implies reasonable hardware connections. — too honest for this site, Sep 18 '18 at 10:43
If you believe that the code works correctly, consider presenting your work (with its unit tests) in a more-complete fashion over at [codereview.se]. You'll likely get some suggestions on making it more efficient, easier to read, and better tested. Before you do that, make sure to read [A guide to Code Review for Stack Overflow users](//codereview.meta.stackexchange.com/a/5778) first, as some things are done differently over there - e.g. question titles should simply say what the code *does*, as the question is always, "How can I improve this?". — Toby Speight, Sep 18 '18 at 10:47

score 2 · Accepted Answer · answered Sep 18 '18 at 07:56

You can read them in one go if they are pins of the same memory-mapped port register. You can then simply create a new mask:

#define SWITCH_ALL (switch_green | switch_yellow | switch_blue | switch_red)

Or harder to read, but otherwise equivalent:

#define SWITCH_ALL 0x0F

Then, assuming you can get rid of the seemingly superfluous pin_read function:

uint8_t button_pressed (void)
{
  return  (uint8_t) (PORTX & SWITCH_ALL);
}

where PORTX is the name of the port data register.

Apart from being faster, this also has the advantage that all your pins will get read synchronously, at the same time.

However, you will naturally need to add some de-bouncing of the button somewhere, or the reads won't be reliable.

bigwillydos · Answer 2 · 2018-09-18T13:57:50.103

-1

Use a struct to organize this information.

typedef struct{
    uint8_t green:1;
    uint8_t yellow:1;
    uint8_t blue:1;
    uint8_t red:1;
    uint8_t :4; //unused
}switch_t;

switch_t s = {0};

// store switch press to val
void button_pressed(switch_t * s)
{
    s->red = pin_read(pin_sw_red);
    //...
    s->green = pin_read(pin_sw_green);
}

button_pressed(&s);
if(s.red){
    //do stuff
}
//...
if(s.green){
    //do stuff
}

edited Sep 18 '18 at 13:57

answered Sep 18 '18 at 01:00

bigwillydos

1,321
1
10
15

2

The first half of your answer is entirely OK. The second half, however, is a bad idea - The compiler is free to allocate the bits in the bitfield whatever order it likes. Setting them through a union is dangerous, and, even if it worked, entirely non-portable. – tofro Sep 18 '18 at 06:39
Indeed, using bit-fields is bad practice in general. In addition, why would you shift the register 5 bits instead of 4? Why do you shift them at all, given that the pins seem to be in the lower nibble? – Lundin Sep 18 '18 at 08:09
@tofro Depends on the compiler and settings used. You can get deterministic behavior out of most compilers with the right settings. – Gerhard Sep 18 '18 at 11:08
@Gerhard Nope, it's not. The allocation of bitfields in a structure is entirely implementation-defined (i.e. the compiler can do what it likes). Even *packing* is optional - Thus there might be bits you can't even reach via a bitfield. The only thing you may safely assume is that what you write through a bitfield can be read back through the same thing - That's about anything. – tofro Sep 18 '18 at 12:35
@Gerhard Actually, one of many issues with bit-fields, namely the alignment of a "storage unit" (piece of the bit-field), is not just implementation-defined, but unspecified behavior. Meaning you can't actually know how a bit-field is allocated on any compiler - the compiler does not need to document it. It is perfectly free to toss in 500 padding bytes between every bit without telling you. In addition, a compiler need not support `uint8_t` bit fields. On and on it goes, I could write a whole essay just about how poorly-defined and problematic they are. – Lundin Sep 18 '18 at 13:01
@tofro thanks for pointing that out. I've removed it from my answer. – bigwillydos Sep 18 '18 at 13:58
@Lundin I was assuming that `// physical pins on microcontroller` followed by `#define pin_sw_green 5` meant bit 5, which is not in first hex digit (nibble). – bigwillydos Sep 18 '18 at 16:50
@tofro & Lundin The compiler can do what they want but the aim is for sensible behavior and it is generally documented. The linked article say more about non atomic operations in a multi threaded environment than the actual use of a union. Yes beware of the limitations and understand what your compiler will do. But compiler designers is not out to get us. – Gerhard Sep 19 '18 at 08:17
@Gerhard Note that SE is a site that is largely visited by newcomers. Recommending bitfields for C development close to the hardware (accessing registers) is IMHO an absolute no-go. Doing that will bite you back sooner or later, the problem here is that this is non-obvious to a beginner and tends to become visible only after you have invested a significant amount of work in a project and need to invest a lot of re-write to fix it. – tofro Sep 19 '18 at 08:33

score -1 · Answer 3 · answered Sep 18 '18 at 09:52

Hiding port access inside a function is only a small and weak form of abstraction. You face the general problem in embedded programming, which is to create a fusion of distinct and distinctly reacting data sources. One way is to create a plethora of individual names (in your case, functions) on the language level - this has a natural limit until maintaining and testing become a nightmare. Such systems tend to die when the originator leaves them, at least they become unmaintained.

Instead try to define the nature of your inputs on the business logic side - in your case especially if they are level or transition - and create a way which allows you to access them with a run-time address (some form of index) instead of a compile-time address (== the function name). You can always organize access in a way which optimizes to equivalently fast code when the parameters are known at compile-time and still keep open the indexed/programmed way. Then write a mapping between your real hardware and these idealized inputs where you can deal with things like debouncing, inverted logic levels, etc.

Trying to condense code while keeping modularity with bitwise operator

3 Answers3