Wallacoloo / printipi

3d printing directly through the Raspberry Pi's GPIO pins
MIT License
141 stars 43 forks source link

Refactor IoPin interface #51

Closed Wallacoloo closed 9 years ago

Wallacoloo commented 9 years ago

The current way GPIOs are handled has a few problems:

  1. Requires anything using an IOPin to be templated for the type.
  2. Cannot change GPIO mappings on-the-fly
  3. Duplicate code in HardwareScheduler & RPiIoPin for changing output states.
  4. Not really a standard interface for implementing GPIOs on other platforms.

To do:

Wallacoloo commented 9 years ago

I think that first off, pins should be treated as data - i.e. instead of having the pin number be a template parameter, have it be a member data field.

Secondly, we don't really need a platform-specific IOPin classes. If you look in HardwareScheduler, it already has a function, void queue(const OutputEvent &evt), and an OutputEvent consists of a pin number, a state (high/low), and a time. So we can just use this to toggle the pin state.

Actually, we do need a platform-specific IoPin class. While the HardwareScheduler DOES have an interface to do writes, it does not have one to do pin reads. Furthermore, it should be possible for a platform to just implement the gpio functionality, and use a default software-scheduling in the future, in which case we would rather have the HardwareScheduler delegate to the IoPin than vice-versa.

I propose a platform-specific gpio interface, which has simple responsibilities of digitalWrite(IoLevel), and then a generic IoPin class that contains extra information like write/read inversions and just provides a slight abstraction above the platform-specific gpio class.

It could look something like:

class IoPin {
    PlatformSpecificPinType pin;
    bool invertReads;
    bool invertWrites;
    IoLevel defaultState;
    public:
        //implementations for the functions already defined in IoPin:
        void makeDigitalOutput(IoLevel);
        void makeDigitalInput();
        IoLevel digitalRead();
        void digitalWrite(IoLevel lev) {
            //just relay the call to `pin`, but invert it if necessary:
            pin.digitalWrite(invertWrites ? !lev : lev);
        }
        ~IoPin() {
            //control the power-off state:
            digitalWrite(defaultState);
        }
};
Wallacoloo commented 9 years ago

It is possible to just make PlatformSpecificPinType an int, and then have the digitalWrite functions, etc, delegate to some static functions. But on certain platforms (even the Raspberry Pi), it may actually make more sense to internally store the pin information as something like:

struct PlatformSpecificPinType {
    char bank;
    char pin;
};

And at that point, it's probably best to make it all object-oriented.

Wallacoloo commented 9 years ago

One downside to removing the templating of pins is that deactivating them on exit becomes slightly more difficult.

The current implementation has the following class:

template <typename ThisT, IoLevel lev=IoLow> class IoPinOnExit {
    //Often times it is useful to ensure that a given pin is put into a given state upon program exit.
    //When this class is instantiated, it inserts a hook into the exit handlers to do just that.
    static void deactivate() {
        ThisT pin;
        pin.digitalWrite(lev);
    }
    public:
        IoPinOnExit() {
            SchedulerBase::registerExitHandler((void(*)())&deactivate, SCHED_IO_EXIT_LEVEL);
        }
};

If we want the new version to use the scheduler in this way, then the scheduler must accept a generic functor, rather than a function pointer.

Another option is to deactivate the IoPin in its destructor, but a call to exit(), signal handlers, etc, can bypass this.

Another option is to keep a static set of references to all living IoPins inside the IoPin class, and register a single exit handler to a static function that enumerates that set & deactivates the pins.

Actually, that last one wouldn't work unless we made the IoPins also set their output to default during destruction, which is undesirable, since they already get copied around a bit. However, we could make the IoPin a non-copiable class, and just move it around.

Alternatively, we could keep a static set of copies all unique IoPins ever instantiated, and then set them to defaults in the exit handler. In the future, we want to move the default values out of the configuration file as much as possible (e.g. it's a given that the hotend should default to off), and this suggests having the IoDrivers (like TempControl) manually configuring the default value by calling a function like ioPin.setDefault(IoLow). This becomes increasingly difficult to synchronize if we allow multiple copies of IoPin.

Wallacoloo commented 9 years ago

Another problem with removing IoPin as a template parameter to other IoDrivers is that it makes it more difficult to control pins in parallel. For example, in the Fan class, we always had the ability to have it drive more than one physical fan simultaneously, through wrapping two pins in a class that exposed the same interface as a pin.

This is no longer possible, but in actuality that's probably a good thing - this is not something that needs to be done very often, and there are certainly better ways of doing it (e.g. have two logically separate fans in the IoDrivers list).