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

Add tilt max and min angle constraints #13

Merged

Conversation

bsell93
Copy link
Contributor

@bsell93 bsell93 commented May 30, 2024

This gives the ability to control max/min value of the angles detected - similar to how "Max Distance" works.

Defaults to -60 to 60deg, which I believe is max detection range; controls are optional.

image

@TillFleisch
Copy link
Owner

Thank you for adding this feature. I only had a quick glance at the PR. Since I currently don't have a lot of free time, a proper review and testing will have to wait (sorry :/).

  • The readme should be updated with the new configuration options
  • The min/max angle should have a margin (like max distance) to prevent flickering on the edges
    • maybe the max distance margin could be reused - this would require renaming and would therefore be a breaking change requiring yaml modification
    • a single new margin for the angle restriction would be sufficient. I don't see why separate margins would be necessary and if so the same effect could be achieved with zones
  • maybe there is some room for consolidation: the min/max distance/angle seem repetitive. Having a single modular number class (and file) would probably be better (I will look into this once I find the time...)

@bsell93
Copy link
Contributor Author

bsell93 commented May 31, 2024

@TillFleisch no worries! I'm happy to make an attempt at contributing 😃

The readme should be updated with the new configuration options

Yep - good call (Edit: Done)

The min/max angle should have a margin (like max distance) to prevent flickering on the edges

I'm actively using this and not seeing any flickering. If we did see a need to add margin I would definitely go the route of making a separate margin for angle rather than re-using distance.

maybe there is some room for consolidation: the min/max distance/angle seem repetitive. Having a single modular number class (and file) would probably be better (I will look into this once I find the time...)

I was literally thinking the same thing when I was writing it, but for the sake of making it work and reducing chance of failure I just duplicated. Very open to this modification though

@TillFleisch
Copy link
Owner

TillFleisch commented Jun 9, 2024

I look over this and added some things, I felt were necessary:

  • The default values now cover a wider area (to not mess with any existing configs)
  • The angle restriction has it's own margin
  • The min/max angles cannot exceed each other (both in config and when using number component during use)
  • I consolidated the number components into a single class

I will look over this once more later today. Feel free to play around/find bugs/criticize

Edit: the config could also be consolidated further. The max distance and angles could be covered by a single schema, which has a type attribute to determine which action is intended. This would break existing configs though...

@@ -52,12 +52,16 @@ namespace esphome::ld2450
ESP_LOGCONFIG(TAG, "LD2450 Hub: %s", name_);
ESP_LOGCONFIG(TAG, " fast_off_detection: %s", fast_off_detection_ ? "True" : "False");
ESP_LOGCONFIG(TAG, " flip_x_axis: %s", flip_x_axis_ ? "True" : "False");
ESP_LOGCONFIG(TAG, " max_detection_tilt_angle: %i °", max_detection_tilt_angle_);
ESP_LOGCONFIG(TAG, " min_detection_tilt_angle: %i °", min_detection_tilt_angle_);
ESP_LOGCONFIG(TAG, " max_detection_distance: %i mm", max_detection_distance_);
ESP_LOGCONFIG(TAG, " max_distance_margin: %i mm", max_distance_margin_);
Copy link
Owner

Choose a reason for hiding this comment

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

angle margin missing

@TillFleisch TillFleisch merged commit e96a3e7 into TillFleisch:main Jun 10, 2024
7 checks passed
@TillFleisch
Copy link
Owner

Thank you @bsell93 for adding this feature! :)

@bsell93
Copy link
Contributor Author

bsell93 commented Jun 10, 2024

Thank you @bsell93 for adding this feature! :)

Thanks for helping me get it across the finish line ☺️

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