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 dead zone to knobs #212

Merged
merged 26 commits into from Jan 20, 2023
Merged

Conversation

redoxcode
Copy link
Contributor

This fixes the issue #209 by implementing deadzones for the knobs.
Please see the discussion in the issue for details.
This PR does not change:

This PR does change:

  • The deadzones are implemented in the percent() function of the AnalogueReader class.
  • Also the Knob class now actually makes use of the percent() function in the AnalogueReader class instead of completely reimplementing that part.
  • Also the _sample_adc() function of AnalogueReader was changed for better performance without changing the results
  • Tests have been modified to account for the deadzones

@awonak
Copy link
Collaborator

awonak commented Dec 26, 2022

Awesome work @redoxcode & @gamecat69! Thanks for taking the time to dig in on this problem. Since this change is modifying current firmware behavior and affects all scripts, I want to make sure we take our time with this change. I've added a few first pass comments. I'll test this change on my EuroPi too when I get a chance.

Copy link
Collaborator

@mjaskula mjaskula left a comment

Choose a reason for hiding this comment

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

Thanks to the submission, the root changes here look pretty good to me!

I've left some comments regarding places that I think that the API, tests, and documentation can be improved. Please let me know if any of my comments are unclear.

software/firmware/europi.py Outdated Show resolved Hide resolved
software/tests/experimental/test_knobs.py Outdated Show resolved Hide resolved
software/firmware/europi.py Outdated Show resolved Hide resolved
software/firmware/europi.py Show resolved Hide resolved
Copy link
Contributor Author

@redoxcode redoxcode left a comment

Choose a reason for hiding this comment

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

all resolved I think

@mjaskula
Copy link
Collaborator

I was still dubious about the need for this change, so I checked this branch out and updated the knob line in the diagnostic script like so:

oled.text(f"{k2.percent():.2f} {k2.percent(deadzone=0):.2f}", 2, 13, 1)

This allowed me to turn the knob and see what value would be retuned both with (L) and without (R) the change. On my hardware the difference was minimal, but noticeable. It turns out that my hardware already exhibits a noticeable deadzone. This is true on both of my modules, one that used 5% resistors, and one that used 1%.

So I guess that my hardware isn't the target demographic for this change, for whatever reason. I think that code changes look good to me now, so I'm going to approve the PR. I think that before we merge this change, it would be nice to have some confirmation from someone with the root problem that this change improves their experience. We might need to tweak the default deadzone value, or make it configurable.

@redoxcode
Copy link
Contributor Author

I think there should be further changes to the tests after adding deadzones to read_position() as well? Or am I wrong here?

@mjaskula
Copy link
Collaborator

I think there should be further changes to the tests after adding deadzones to read_position() as well? Or am I wrong here?

Yeah, good call, should be very similar to the other test additions.

@redoxcode
Copy link
Contributor Author

Yeah, that's why I didn't mark that part as resolved yet. Just haven't had the time yet.
I guess the tests don't fail cause the deadzones are to small to have an effect in most cases.

@mjaskula
Copy link
Collaborator

Yeah, that's why I didn't mark that part as resolved yet. Just haven't had the time yet.

No worries, take your time. There's no rush here.

@redoxcode
Copy link
Contributor Author

ok, think I included deadzones of 0.01 (default) and 0.0 in all the tests now.
Also according to #209 this fixes the issue for people that had it, while not introducing a noticeable difference for people that don't (from my tests).
Anyway the exact default value could be tuned by future PRs.
So.. ready to merge?

@mjaskula
Copy link
Collaborator

I think that before we merge this change, it would be nice to have some confirmation from someone with the root problem that this change improves their experience.

I've reached out to a few people for some more testing on this.

Other than that the code changes look good to me, and my approval stands. We can merge as soon as we get another set of eyes on it.

Copy link
Collaborator

@awonak awonak left a comment

Choose a reason for hiding this comment

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

Everything looks good to me. Thanks for all your hard work and clean code with this issue!

@gamecat69
Copy link
Contributor

Nice work! I can confirm this PR successfully addresses the noise issue on the knobs using the deadzone approach. However, the noise on ain also mentioned in #209 is still present. See more info on how I tested this below. I suspect the noise on ain is still present because this PR only contains the knob deadzone code (as in the title), but I didn't go any deeper into the code to confirm. Suggest the issue remains open until the ain noise is also addressed.

I used the latest version of all packages and copied over the new europi.py.
I then ran this script to test the knob min and max values and ain min values using the previous and new version of europi.py, see results below.

Previous version of europi.py:
k1 min: 0.000366
k2 max: 0.997284

k2 min: 0.000366
k2 max: 0.997269

ain low: 0.004055

Version of europi.py in this PR:
k1 min: 0.0
k2 max: 1.0

k2 min: 0.0
k2 max: 1.0

ain low: 0.004078

@redoxcode
Copy link
Contributor Author

Yes, this PR only addresses the issue with the Knobs. The deadzone code could be activated for ain in an other PR.
However I don't think deadzones are the way to go for ain.
I opened a new issue for the ain here: #211

@mjaskula mjaskula merged commit d58989a into Allen-Synthesis:main Jan 20, 2023
@redoxcode redoxcode deleted the add-dead-zone-to-knobs branch January 23, 2023 11:24
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.

None yet

4 participants