Lyr3x / Roode

A reliable smart home people counter based on VL53L1X and ESPHome
The Unlicense
149 stars 41 forks source link

Refactor configuration structure #90

Closed CarsonF closed 2 years ago

CarsonF commented 2 years ago

This new structure breaks things up by subject instead of the manual/automatic parameter values. I believe this is much easier to understand and allows parameters to be more granularly defined with automatic or manual values.

New minimal config with defaults

roode:

All options spelled out

# Sensor is configured separately from Roode people counting algorithm
vl53l1x:
  calibration:
    ranging: longest # defaults to "auto" for formerly "calibration: true"
    offset: 8mm
    crosstalk: 53406cps

  # These pins are not yet implemented but they are passed into class now
  pins: 
    xshut: 3 # shutdown pin to change address or multiple sensors
    interrupt: 1 # hardware callback when measurement is ready

roode:
  # I removed the { size: 1 } option here since it was redundant.
  # Can always add back later if we have more sampling paramaters.
  sampling: 1

  # defaults for both zones
  roi:
    height: 14
    width: 6

  detection_thresholds: # defaults for both zones. These also default to 0% & 85% as previously done.
    min: 234mm # absolute distance
    max: 85% # percent based on idle distance

  zones:
    invert: true

    entry:
      roi: auto # formerly "roi_calibration"

    exit:
      roi:
        height: 4
        width: 4
        center: 124
      detection_thresholds:
        max: 70% # override max for exit zone

I tried to not touch too much, but it was hard. One of my main goals was to get stuff out of Roode class. So having separate Zone, Threshold, ROI objects helps with that. But still configuring everything through Roode defeats that purpose.

There's a lot of data points here. I avoided a lot of getters & some setters because it's just so much boilerplate that doesn't add any value. Usually it's said to be "cleaner" because "encapsulation", but it's really not. Data set via property or setter is the same thing, the setter doesn't restrict anything. I have them in a few places where it's easier to code gen in python. As we continue to refactor we can reduce the tightness of the coupling between these objects. Just trying not to do too much at once here.

CarsonF commented 2 years ago

I like the addition of orientation and ranging as well as the move of the config to the zone. However, reading this structure i feel like it might make sense to add a sensor object which holds all sensor specific settings? I know i previously denied that, but at the current state it makes sense to build a Sensor in Roode instead of zone, orientation and ranging. What do you think? Of course we can skip that to another PR anytime soon.

Yeah I totally agree. Was it was coming together I thought that as well. I can do it here if you want or we can wait. Up to you.

  • Cause you are very energetic to ditch getter/setter -> lets do so

I don't want to bully you into it lol. I do feel strongly about it and I tried to post some reasoning. I definitely want to look into immutability or blocking writes where not expected. But also given the nature of how the codebase is consumed it might not be too important...

  • Your C++ auto-formatter is configured different as mine, which sucks because there is a lot of formatting in this PR. Ill add a auto-formatter config to the project to prevent that.

I see you've added a clang formatter config. I was missing that as well. The formatting was inconsistent but I didn't want to do a blanket reformat of code I wasn't touching. Also I wasn't sure which preferences you had. I'll rebase and reformat πŸ‘

CarsonF commented 2 years ago

Rebased & got new formatting in. More tomorrow.

CarsonF commented 2 years ago

There's still lots of clean up that can happen. But since this is blocking you, I'll stop here πŸ˜„

Lyr3x commented 2 years ago

There's still lots of clean up that can happen. But since this is blocking you, I'll stop here πŸ˜„

This is a great PR! I'll need to test it in detail before merging it, but it looks good to me. Feel always free to open another PR πŸ™‚