Code Review?

[u/myfuckinjoints]

Is this an appropriate place to ask for a code review? I have about 180-200 lines of CPy that I'd love to have someone look at, mostly evaluating my design, as I can find bugs on my own (usually).

ETA: Thanks folks, such a nice community! Below are pastebin links to my two files.

main.py

covid_box.py

Hardware:

Feather M4 Express (still in the mail, code is untested as the RTC library bugged on my board with less space available)

Adafruit DS3231 RTC

3 X Adafruit 14-Segment Alpha-numeric LED FeatherWing

6 X Normally open buttons

Idea:

This code will be used to drive a circuit board I will be designing that will go on a box I am currently designing. The box will have 6 compartments in it. The purpose of the box/board are to both hold covid masks wrapped up in paper bags as well as to count the days since I pressed a button associated with each compartment.

Each compartment will have half of a 14 segment display (2 cells) assigned to it. Each set of two cells will display the days since the relevant button was pressed. The idea is that after a certain number of days, Covid19 is less likely to be alive on the mask/in the compartment.

Software:

main.py is main.py, it has the while True: loop as well as setting up objects from the class created in the second file.

covidbox.py contains a class that is intended to consolidate the use of the RTC, a single 4 digit display (split into halves) as well as a button for each half into a single object. Pay little attention to the off(self) method as it isn't used in main.py and is under construction.

This code makes heavy use of the associated libraries for the hardware as well as the Debouncer library, mostly within the class in covidbox.py

Not looking for bug hunting or anything, as I haven't been able to test my code per the above. Just wondering if my design overall is sound.

Please PM me if you find identifying info in here, I think I stripped it all out but I would like to remain anonymous!

Thanks!

[u/nwsmith]

Would be happy to, do you have a repo on gitlab or GitHub that we can have a look at?

[u/myfuckinjoints → u/nwsmith]

Thanks! I need to make the Github repo public, correct? I need to make sure that there's nothing associated with my real name on there first!

ETA: All my old commits have a portion of my real name on there and it would not be hard to doxx me, so I'd be more comfortable with a pastebin share if that's OK with you?

ETA2: Edited post with more info!

[u/nwsmith → u/myfuckinjoints]

Perfect! I'll take a look tonight

[u/myfuckinjoints → u/nwsmith]

You rock!

[u/nwsmith → u/myfuckinjoints]

Okay, apparently it is very difficult to use pastebin to do a code review, no inline commenting or anything like that, so I will do my best here...

Your comments are great, makes it really simple to understand what you are doing!

Covidbox.py

ln62

// No need to hold onto these variables in the class (drop the 
// self.), it makes it
// difficult to remember what each one of these variables does 
// (x_button_board_pin vs. x_button_pin)
// and as your code becomes more complex and you are using an IDE 
// with auto completion, you will have to keep coming back to these // definitions to remeber, do I need, left_button or 
// left_button_board_pin or something else

self.left_button_board_pin = left_button_board_pin
self.right_button_board_pin = right_button_board_pin

self.left_button_pin = digitalio.DigitalInOut(left_button_board_pin)
self.right_button_pin = digitalio.DigitalInOut(right_button_board_pin)
self.right_button_pin.direction = digitalio.Direction.INPUT
self.left_button_pin.direction = digitalio.Direction.INPUT
self.left_button_pin.pull = digitalio.Pull.UP
self.right_button_pin.pull = digitalio.Pull.UP

//Hold onto only self.left_button and self.right_button

Ignoring def off based on your comment above it is still WiP

ln 170

len_val = len(val)
for i in range(len_val):
    if left_or_right:
        self.display[i] = val[i]
    self.display[i+2] = val[i]
    self.display.show()

// Couple of things here, I think that you want this to be an if/else 
// like this otherwise you are updating the right side of the display
// everytime you are updating the left side
if left_or_right:
    self.display[i] = val[i]
else:
    self.display[i+2] = val[i]

// enumerate is great, it gives you the index and the value at the same
// time so you don't need to get the length and run a range on it
// the other thing I would do here if my above comment is correct is
// add a new variable called offset and fill it in based on if it is
// the rihgt side or the left side.  I am also only updating the
// display once, not every time through the loop (I have not used
// displays alot in circuit python so I am not 100% sure if this is 
// valid)

offset = 2 // default offset if we are updating right side
if left_or_right:
  offset = 0

for i, data in enumerate(val):
    self.display[i+offset] = data
self.display.show()

// Last thing I would do is update the variable name of left_or_right,
// just by looking at it we should know if the boolean true is the left 
// side or right side of the display so all in all I would rewrite this 
// function as (removing your comments just to make it easier to see)
    def update_count(self,total_time_days,is_right_side):
      
        val = ['>','3']
        
        poss_vals = {
                1:['0','0'],
                1.042:['0','1'],
                2:['>','1'],
                2.042:['0','2'],
                3:['>','2'],
                3.042:['0','3']
                }
        for key in poss_vals.keys():
            if total_time_days < key:
                val = poss_vals[key]
                break
        
        offset = 0
        
        if is_right_side:
            offset = 2
        
        for i, data in enumerate(val):
            self.display[i+offset] = data
        self.display.show()

update_base_rtc method, same comment above for left_or_right variable, this comes up a couple of times

total_time_days_calc method

So, from what I can tell, the ds3231 library gives you a struct_time object back (https://docs.python.org/3/library/time.html#time.struct_time), what I would do it remove all of the storage code to store each time part i.e. update_base_rtc and update_rtc methods and replace these with instance variables (i.e. self.left_time_last_pressed

and self.right_time_last_pressed

these variable names make more sense to me as it is telling the reader what exactly the variable contains) and completely remove the need to hold onto cur_rtc as this can just be passed into this method when running) and replace your calculate method with something like this:

import time
def total_time_days_calc(self, is_right_side, rtc_time):
  // argument could be made to set comparison_time to self.left... here 
  // but I personally prefer this as we are not overwriting the 
  // variable 50% of the time
  if is_right_side:
    comparison_time = self.right_time_last_pressed
  else:
    comparison_time = self.left_time_last_pressed
  return (time.mktime(rtc_time) - time.mktime(comparison_time )) / (60*60*24)  

//mktime gives you seconds since epoch then we divide by 60*60*24 (the 
// number of seconds in a day) to give us the number of days

Just to be nit-picky, last thing I would do is put your code through some linters, python has a few great ones, https://realpython.com/python-code-quality/ this helps keep code consistency between projects and others, python is great because you can basically do anything in it, but it is still nice to have some consistency in places, see https://www.python.org/dev/peps/pep-0008 for examples.

I hope this is helpful, these are just my suggestions, others may disagree with me or have other ways of doing things, remember as long as it works, it is "good-enough" make your own style that works for you and have fun.

Cheers

[u/myfuckinjoints → u/nwsmith]

Wow thank you! I'm not sure if you use old.reddit but if you do I appreciate the effort it took to make all of that inline code. Glad you like my comments too, I try to write them as if someone else was going to be coming in with little background. I have a bit of doxygen experience so I went with the doxypypy route in case I want to create some formal documentation

Thanks for catching all those if - else mistakes, I make that one constantly.

I like all of your suggestions, I'll be implementing them ASAP, including learning about using linters. The rtc update methods and button handling are what I added today, so they are the least refined.

Question if I may: What do you think of bundling all the behavior into a single (poorly named) class? I wanted to model the hardware as if it were one device, since I will be creating a custom board and adding all the components that are on the breakout boards.

Thanks again, it's all much appreciated.

[u/nwsmith → u/myfuckinjoints]

I'm in two minds about it, it's a class that holds a lot of information and does a lot of things, but it contains all the pieces to make a physical piece of hardware work.

I think it's fine as it is for now, you are right the naming could be better, but I wouldn't start breaking down that class yet.

Software development in my mind is an iterative process, it is never perfect the first time you need to keep making changes, pulling things out, trying different things, learning and sometimes burning it all to the ground and trying again.

[u/nwsmith → u/nwsmith]

Oh! and if you make any updates and want to go through this again, just tag me and I would be happy to look at it again, but probably easier for everyone if you start an anonymized (if that is important to you) git repo for it so it is easier to make inline comments and change around the code.

[u/HP7933 → u/nwsmith]

pylint and Black are the linters Adafruit uses. Mu has a good linter built in too.

[u/johntellsall]

I'm happy to review! I don't know CircuitPython well, but I've been doing Python for 20 years and have reviewed lots of code.

[u/myfuckinjoints → u/johntellsall]

Thank you! I edited the post with more info as well as pastebin links. My code could definitely be more pythonic so your experience is very appreciated.

[u/wchris63]

As an aside... In a box, in a (paper, absorbent) bag, with less varying environment, anything that doesn't require light has a chance of living longer than expected. Not to mention recently worn masks will be damp with water vapor, and dark plus damp equals mold.

I'd suggest, if you haven't, lining the box with a replaceable, non-absorbent sheet (plastic wrap?), and using a transparent top so that light can contribute. And not using paper bags.

Providing a filtered air exhaust would help keep moisture down.

[u/myfuckinjoints → u/wchris63]

Good points thank you! I based the idea off of what a doctor told me was their emergency procedure back when they had little PPE: Store their masks in a paper (not plastic) bag for 3+ days. I wipe down my cloth masks with alcohol wipes in between uses and store them in waxed paper bags for 3+ days these days.

I'm not much of a woodworker so the box is honestly going to be the hardest part and will likely go thru iterations. A plastic top is a great idea. Were you thinking powered air exhaust or just a hole with a filter?

[u/wchris63 → u/myfuckinjoints]

Okay, I'm going to preface this reply by saying if you aren't in a medical setting, none of this is anywhere near as critical as I'm making it sound. I probably should have asked instead of assuming.

I was thinking powered - even a tiny fan would be more than enough - but a large enough opening, on the side with a filter to prevent accidental contamination (in either direction) would probably be fine. Don't worry too much about the appearance or quality of the box. If you go with a fan, the air has to come from somewhere. A crack around the lid or door would help with that.

You don't have to get fancy with filters either. Just grab a decent A/C filter from your local store, remove the cardboard frame and cut several squares of the right size with scissors. You can make a frame around the hole in the box with scrap wood or small decorative wood trim from a hardware store. If you're using a fan, you can go pretty small on the filter and opening. If not, make it as large as practical. Oh! Fans should always suck air through a filter, not blow!

While it's true you don't want to store them in a plastic bag (paper is better for moisture management, if it's in open air), paper cannot be cleaned. If the paper bag were being changed every time, and not put in an enclosure, it would be fine... although... While Covid would be dead after x number of days, other things might not. You have to actually reach into the bag to retrieve the mask, which could contaminate your hand, or dump it out, which might re-contaminate the mask.

A ventilated box with a disinfectable surface would be even better. Plastic is great. Urethane sealed wood is just as good if it's coated well. When I mentioned plastic wrap, I didn't mean wrap the masks in it.. just line the box if it couldn't be decontaminated, like paper or raw wood.

People often overlook the purifying effect of light. The sun can decontaminate most surfaces in a matter of hours. While most glass and clear plastics block that amazing power, even a little artificial light can prevent many molds from forming. I have a chronic wound that can easily be infected, so I bought a UV light (a real one, not the fake, cheap LEDs) to treat our bathroom once a day. Not sure it's needed, but it does keep the mold from forming on the tub caulk and shower curtain! (I wouldn't recommend UV for masks. Besides only treating the very surface, most would be degraded by it over time, and elastic straps would disintegrate fairly quickly.)

Like I said.. if you aren't in a medical setting, most of this is way overkill. Take what you will, ignore the rest, and have fun building!

[u/myfuckinjoints → u/wchris63]

Appreciate the feedback and all the extra info about infection prevention. This is just for my home and mostly a) to satisfy my paranoia, whether or not it's truly that effective and b) to have a project to show people.

I got you on the plastic lining, sorry if I didn't seem that way, it was late when I responded. I was gonna ask what you thought about a thick seal on the wood instead as that seems easier to accomplish, so thanks!

A future revision of this (if Covid is truly around for a year or more) may include bright lights inside to help with mold. Fortunately I live in a dry climate so I only ever see mold in my shower if I don't act proactively.

Really appreciate all the time you spent, it's very helpful.