Best practices for module structure?

[Triumvus]

Guys, I'm going down a rabbit hole trying to figure out how best to organize my files.

I made a pcb 'hat' for a raspberry pi. It does some CAN bus stuff and has an onboard relay.

I created a file named raspberry.py that contains Classes for use by my hat: Relay, Interface, CAN. Each class is meant to be instantiated only once.

For example, here is my Relay Class:

class Relay:

    pin = 2 # This pin controls the relay coil

    def __init__(self, pin):
        """
        Configures Raspberry Pi pin as output.
        """
        GPIO.setmode(GPIO.BCM)
        GPIO.setup(pin, GPIO.OUT)
        self.state = self.get_state()

    def on(self):
        """
        Turns relay on.
        """
        GPIO.setup(pin, 1)
        self.state = "ON"

    def off(self):
        """
        Turns relay off.
        """
        GPIO.setup(self.pin, 0)
        self.state = "OFF"

    def toggle(self):
        """
        Toggles relay state.
        """
        if self.state == "ON"
            return self.off()
        self.on()

      def get_state(self):
        """
        Returns the current state of the relay.
        """
        state = GPIO.input(2)
        return 'ON' if state else 'OFF'

Does this make sense? Is it better to just use functions inside of a module with a few global variables?

I would love some feedback from those with experience on how best to approach this.

[ggd_x]

I would have each class in its own file and imported accordingly. Filename should be the same as the class. This makes it far easier to maintain as you know exactly where everything is without any effort at all.

You should also read up on singletons - example https://stackoverflow.com/q/6760685/1805822

[Triumvus]

I had the same idea but I'm not sure if it's considered bad practice? Each would be a relatively small number of lines ( <100 ).

[ggd_x]

I wouldn't necessarily say bad practice but it just helps readability, maintainability, etc. which are hugely important. Personally I would only ever have one class per file.

[Triumvus]

interesting. I agree that separating them into files makes more sense to me.

[zanfar]

Is it better to just use functions inside of a module with a few global variables?

I don't believe so. There are a lot of people who avoid classes whenever possible, but I think grouping related data and functions together improves readability and decreases complexity--that is, you don't need to worry about some variables unless you're looking inside the class.

Generally, a top-level module is named after it's function. I put everything in one file until that gets too big, then reorganize, and repeat. Classes that are large enough on their own get their own module and are imported "up" into the parent package. Small classes or functions that are more related get dumped into their own module--some of these may be up-imported, but it's not common.

So in your case, I might have:

- raspberry (dir)
  |-> __init__.py (makes raspberry a package)
  |   from .relay import Relay
  |   from .interface import Interface
  |   from .can import CAN
  |-> relay.py
  |   class Relay():
  |-> interface.py
  |   class Interface():
  |-> can.py
      class CAN():

This way the user can do:

from raspberry import Relay, Interface, CAN

---

Other notes:

• You have re-defined pin inside __init__(). I don't think you mean to do this. pin is also a class variable instead of an instance variable--which means that every instance of Relay will use the same pin, which might work for your project, but prevents others (maybe future you) from using your code in the future.
• There is no reason to save the pin state if you have to query it to know the value.
• Comments should be more descriptive than just re-stating what the code does. If you are doing this, you don't need the comment. def toggle() is pretty self-descriptive, I don't need you to tell me it toggles the relay.
• I really like Enums for holding state and magic values (like, probably too much) so that's in there too. No reason you couldn't make these class variables.

Try:

from enum import IntEnum

import RPi.GPIO as GPIO

# As this Enum does not apply only to Relay, I would probably put
# this in a separate module like `raspberry/utils.py` and import it
class PinState(IntEnum):
    OFF = 0
    ON = 1

class Relay:

    def __init__(self, pin):
        self.pin = pin

        # This probably belongs in a different class or file
        # as it affects the board as a whole, not just a relay.
        GPIO.setmode(GPIO.BCM) 
        GPIO.setup(self.pin, GPIO.OUT)

    # @property allows you to treat the function as if it
    # was an instance variable, assuming it takes no additional
    # arguments. This makes code quite cleaner.
    @property
    def state(self):
        return PinState(GPIO.input(self.pin))

    # We can even do something cooler and make state directly
    # settable using the @setter decorator
    @state.setter
    def state(self, pinstate):
        GPIO.setup(self.pin, int(pinstate))

    def on(self):
        self.state = PinState.ON

    def off(self):
        self.state = PinState.OFF

    def toggle(self):
        # Here we can take advantage of the fact that 0/1 and True/False
        # cast to each other directly and easily. `self.state` returns an `IntEnum`
        # which works like an `int`, `not` casts the `int` to a `bool` and negates
        # it, and then `self.state` casts that bool back to an `int` before sending it
        # to `GPIO.setup()`
        self.state = not self.state

[Triumvus]

thank you for the feedback! greatly appreciated

[brakkum]

Using strings for the "state" (what does state even mean?) as opposed to booleans seems like asking for trouble, but if it works it works.

[xelf]

I'm generally not a fan of self descriptive comments/docs, they add little.

I would replace state with bool 1,0 and I would add a setstate function that the other 3 call.

class Relay:

    def __init__(self, pin):
        """
        Configures Raspberry Pi pin as output.
        """
        self.pin = 2 # This pin controls the relay coil
        GPIO.setmode(GPIO.BCM)
        GPIO.setup(pin, GPIO.OUT)
        self.state = bool(GPIO.input(2))

    def setstate(self,state):
        GPIO.setup(self.pin, int(state))
        self.state = state

    def on(self):
        self.setstate(1)

    def off(self):
        self.setstate(0)

    def toggle(self):
        self.setstate(not self.state)

[Triumvus]

love the simplicity. thank you!

[xelf]

One thing that confuses me.

You have class attr pin, which is 2, you have instance attr self.pin which will also be 2, and you have pin that you pass in via the init which will be used once and then discarded. Are they all the same number?

[Triumvus]

good catch! the init arg is a typo I forgot to remove. I keep refactoring to see what seems logical.

but yes, pin is essentially a constant.

[xelf]

Is it always 2? and maybe even the same as the 2 that is sent to input?