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

feat: add speed factor sensor #208

Merged

Conversation

TheJefe
Copy link
Contributor

@TheJefe TheJefe commented Sep 20, 2023

Description

This PR adds a simple speed_factor sensor
API Documentation: https://www.klipper3d.org/Status_Reference.html#gcode_move

Additionally, It is maybe worth noting that I am using a Sonic Pad, and having no issues with this project

Testing

Tested on my local HA setup, works exactly as expected

image

Other

Not included in this PR, I added some macros and buttons to adjust the speed factor
image

@marcolivierarsenault
Copy link
Owner

@TheJefe please 👁️ the tests

@TheJefe
Copy link
Contributor Author

TheJefe commented Nov 18, 2023

@marcolivierarsenault , sorry for the delay, but I fixed the tests

@marcolivierarsenault
Copy link
Owner

@TheJefe

To fix formating make sure pre-commit hook works.

else you can run black manually black .

Copy link
Owner

@marcolivierarsenault marcolivierarsenault left a comment

Choose a reason for hiding this comment

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

Make sure you have black pass and

add some test for your new sensors.
(AKA, add parameters to test_sensors in test_sensors.py file.)

adding

        ("mainsail_speed_factor", "200.0"),

should do the trick

@TheJefe
Copy link
Contributor Author

TheJefe commented Nov 18, 2023

Thanks for the guidance @marcolivierarsenault ! I've made the requested updates

@marcolivierarsenault
Copy link
Owner

This look great, will do final review and merge later this weekend

@marcolivierarsenault marcolivierarsenault merged commit 62f3775 into marcolivierarsenault:main Nov 25, 2023
5 checks passed
@marcolivierarsenault
Copy link
Owner

Thank you @TheJefe

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