HiveBrain v1.2.0
Get Started
← Back to all entries
patterncMinor

74HC595 IC software implementation

Submitted by: @import:stackexchange-codereview··
0
Viewed 0 times
implementationsoftware74hc595

Problem

I have started to reading up on electronics and embedded software. Lately I have been looking at IC's, particularly shift registers. I thought a good way of learning more about serial communication and shift registers was to implement the internal logic of a shift register.

The one I have chosen is the 74HC595.

I have done this so that it will be similar to working with setting different CPU pins to HIGH or LOW. Imagine that each of the functions manipulating the IC is the value of a CPU pin connected to the IC.

Of course this can be simplified by a easy to use wrapper, so you could just call a function saying which pin to set to LOW or HIGH on the register. But that's not the main point.

ic_example.c:

#include 
#include "../src/ic/ic_74hc595.h"

int main()
{
    struct ic_74hc595 ic = {0, 0, LOW, LOW, LOW};

    /* Initial value check. Should be 0 */
    printf("ic value: %u\n", ic.value);

    /* Set the first bit to 1. */
    ic_set_stcp(&ic, HIGH);
    ic_set_ds(&ic, HIGH);
    ic_set_stcp(&ic, LOW);
    printf("ic value: %u\n", ic.value);

    /* Iterate to third bit */
    ic_set_shcp(&ic, HIGH);
    ic_set_shcp(&ic, LOW);
    ic_set_shcp(&ic, HIGH);
    ic_set_shcp(&ic, LOW);

    /* Set it to 1 */
    ic_set_stcp(&ic, HIGH);
    ic_set_ds(&ic, HIGH);
    ic_set_stcp(&ic, LOW);
    printf("ic value: %u\n", ic.value);

    /* Iterate to last bit */
    ic_set_shcp(&ic, HIGH);
    ic_set_shcp(&ic, LOW);
    ic_set_shcp(&ic, HIGH);
    ic_set_shcp(&ic, LOW);
    ic_set_shcp(&ic, HIGH);
    ic_set_shcp(&ic, LOW);
    ic_set_shcp(&ic, HIGH);
    ic_set_shcp(&ic, LOW);
    ic_set_shcp(&ic, HIGH);
    ic_set_shcp(&ic, LOW);

    /* Set it to 1 */
    ic_set_stcp(&ic, HIGH);
    ic_set_ds(&ic, HIGH);
    ic_set_stcp(&ic, LOW);
    printf("ic value: %u\n", ic.value);

    /* Reset */
    ic_set_mr(&ic, LOW);
    printf("ic value %u\n", ic.value);

    return 0;
}


ic_74hc595.h:

```
#ifndef IC_74HC595_H_
#define IC_74HC595_H_

#include

#define HIGH 1

Solution

OK. Looking at this as a code maintainer and not understanding what the underlying logic of the chip does (which is what will happen in real life).

The function names look a bit generic.

You may want to prefix them with the name of the chip (C unlink C++ does not provide function overloading). So that you can provide similar functions to a set of chips.

From reading the docs this looks good (though I must admit I am not 100% sure).

But a general maintainer is going to look and wonder why half the code paths do nothing.

void ic_set_shcp(struct ic_74hc595 *ic, bit shcp)
{
    if (ic->shcp == LOW && shcp == HIGH) {
        ic->shcp = HIGH;
        ic->i++;
    }else if (ic->shcp == HIGH && shcp == LOW){
        ic->shcp = LOW;
    }
}


So I think the function deserves more documentation (especially why the non state change if ic->shcp == shcp).

Can the next function just not be simplified (I may be missing something in which case a comment):

void ic_set_stcp(struct ic_74hc595 *ic, bit stcp)
{
    if (ic->stcp != stcp) {
        ic->stcp = stcp;
    }
}


Is this not the same as:

void ic_set_stcp(struct ic_74hc595 *ic, bit stcp)
{
    ic->stcp = stcp;
}


OK Reading the function table (page 3) in the linked document:

FUNCTION TABLE See note 1.

I see a set of input and a set of outputs. I also see that the outputs depend on a change in the state of the inputs. As a result I would expect my the functions to mirror this table (maybe this is a bit low level?).

Thus I would have expected an interface like this:

typedef enum { Low, NoCharge, High, HighImpedence} PinState; 
  typedef enum { Q0, Q1, Q2, Q3, Q4, Q5, Q6, Q7, Q7Dash} OutputPinID;
  struct ic_74hc595   { /* Internal definition */ };

  /*
   * The return value of ic_74hc595_change_inputs() returns an encoded form of the
   * output pins. You can use ic_74hc595_decodeOutputValue() on this returned value
   * to decode individual pins.
   */
  void ic_74hc595_power_up(ic_74hc595* ic);
  int  ic_74hc595_change_inputs(ic_74hc595* ic,
                           PinState SH_CP, PinState ST_CP, 
                           PinState OE,    PinState MR, PinState DS);
  PinState ic_74hc595_decodeOutputValue(int QSlots, OutputPinID pin);

Code Snippets

void ic_set_shcp(struct ic_74hc595 *ic, bit shcp)
{
    if (ic->shcp == LOW && shcp == HIGH) {
        ic->shcp = HIGH;
        ic->i++;
    }else if (ic->shcp == HIGH && shcp == LOW){
        ic->shcp = LOW;
    }
}
void ic_set_stcp(struct ic_74hc595 *ic, bit stcp)
{
    if (ic->stcp != stcp) {
        ic->stcp = stcp;
    }
}
void ic_set_stcp(struct ic_74hc595 *ic, bit stcp)
{
    ic->stcp = stcp;
}
typedef enum { Low, NoCharge, High, HighImpedence} PinState; 
  typedef enum { Q0, Q1, Q2, Q3, Q4, Q5, Q6, Q7, Q7Dash} OutputPinID;
  struct ic_74hc595   { /* Internal definition */ };

  /*
   * The return value of ic_74hc595_change_inputs() returns an encoded form of the
   * output pins. You can use ic_74hc595_decodeOutputValue() on this returned value
   * to decode individual pins.
   */
  void ic_74hc595_power_up(ic_74hc595* ic);
  int  ic_74hc595_change_inputs(ic_74hc595* ic,
                           PinState SH_CP, PinState ST_CP, 
                           PinState OE,    PinState MR, PinState DS);
  PinState ic_74hc595_decodeOutputValue(int QSlots, OutputPinID pin);

Context

StackExchange Code Review Q#7638, answer score: 5

Revisions (0)

No revisions yet.