Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor configuration structure #90

Merged
merged 26 commits into from
Jan 15, 2022
Merged

Conversation

CarsonF
Copy link
Collaborator

@CarsonF CarsonF commented Jan 10, 2022

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:
  • vl53l1x section can be omitted if using defaults
  • i2c section can be omitted if on default pins SDA/SCL

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.

Copy link
Owner

@Lyr3x Lyr3x left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Two sidenotes:

  • Cause you are very energetic to ditch getter/setter -> lets do so
  • 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.

components/roode/__init__.py Outdated Show resolved Hide resolved
components/roode/ranging.h Outdated Show resolved Hide resolved
components/roode/ranging.h Outdated Show resolved Hide resolved
@CarsonF
Copy link
Collaborator Author

CarsonF commented Jan 11, 2022

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 👍

Comment on lines 34 to 35
while (1)
{
}
this->mark_failed();
return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you know about this and avoided it for a reason? API states it logs the error and then update/loop/etc. functions are no longer called. This felt better than blocking the entire runtime.

@CarsonF
Copy link
Collaborator Author

CarsonF commented Jan 11, 2022

Rebased & got new formatting in. More tomorrow.

components/roode/roode.cpp Outdated Show resolved Hide resolved
}
),
}
).extend(i2c.i2c_device_schema(0x52))
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
).extend(i2c.i2c_device_schema(0x52))
).extend(i2c.i2c_device_schema(0x29))

When i added 0x52 i was using a knock off sensor. Genuine VL53L1X have 0x29 as default address

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Owner

@Lyr3x Lyr3x Jan 14, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope the pololu as well as the GY-53 uses 0x29 and those are the two preferred sensors. Black PCB sometimes uses 0x52. ST itself says 0x29 7 Bit or 0x52 16 bit.

I have both variants here, however I will only support the polol, guy-53, adafruit and sparkfun

…ult to first found.

This lets the entire VL53L1X component be omitted from config if using defaults
…ROI.

This allows ROI to be recalibrated later without losing what the config overrides are.
Don't set ranging mode again if it hasn't changed.
@CarsonF CarsonF marked this pull request as ready for review January 14, 2022 02:08
@CarsonF
Copy link
Collaborator Author

CarsonF commented Jan 14, 2022

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

@CarsonF CarsonF requested a review from Lyr3x January 14, 2022 02:10
@Lyr3x
Copy link
Owner

Lyr3x commented Jan 14, 2022

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 🙂

@Lyr3x Lyr3x merged commit 8e1c403 into Lyr3x:refactor-configuration Jan 15, 2022
@CarsonF CarsonF deleted the config branch January 15, 2022 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants