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 config files for Recore A6, A7 and A8 #6567

Closed
wants to merge 4 commits into from

Conversation

eliasbakken
Copy link
Contributor

The ina381 is a current monitor circuit with over current alarm. In order for Recore boards to work, this circuit needs a reset and that is what this plugin does.

Signed-off by: Elias Bakken [email protected]

The ina381 is a current monitor circuit with over current alarm.
In order for Recore boards to work, this circuit needs a reset and that is what this
plugin does.

Signed-off by: Elias Bakken <[email protected]>
Copy link
Collaborator

@KevinOConnor KevinOConnor left a comment

Choose a reason for hiding this comment

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

Thanks.

FYI, I can review code modules, and James reviews config files. So, it can be a little challenging to mix them in the same PR.

See my code comments below.

-Kevin

ppins.register_chip('ina381', self)

# Reset over current alarm
reset_pin = config.get('reset_pin', 'ar100:PF4')
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would not make sense to use a default that contains a user specified mcu name.

reset_pin = config.get('reset_pin', 'ar100:PF4')
oc_reset = ppins.setup_pin('digital_out', reset_pin)
mcu = oc_reset.get_mcu()
pin = oc_reset._pin
Copy link
Collaborator

Choose a reason for hiding this comment

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

Code should not "peek" into class member variables of classes defined in other python files - this is mentioned briefly at https://www.klipper3d.org/Code_Overview.html#adding-a-host-module .

@KevinOConnor KevinOConnor added the pending feedback Topic is pending feedback from submitter label Apr 27, 2024
…ins suggestions. This is now a config PR only.
@eliasbakken eliasbakken changed the title Add modules ina381 and add config files for Recore A6, A7 and A8 Add config files for Recore A6, A7 and A8 May 2, 2024
@eliasbakken
Copy link
Contributor Author

I've implemented this PR as a pure config file change as pr your suggestion.

@KevinOConnor KevinOConnor removed the pending feedback Topic is pending feedback from submitter label May 14, 2024
@KevinOConnor
Copy link
Collaborator

Thanks. In general, it seems fine to me. At a minimum the configs need to be added to the build test cases though ( https://www.klipper3d.org/Example_Configs.html point 4).

@JamesH1978 - do you have any comments?

-Kevin

@JamesH1978
Copy link
Collaborator

Config looks fine, my only main comment would be why you have such a low min_extrude_temp and a min_temp of 1 kelvin for your extruders?

Once the regression tests are added as per Kevin's comment, i will do a few review.

Thanks
James

@eliasbakken
Copy link
Contributor Author

Thank you both. I've added the config files as tests, but in order to make them pass I had to make some code changes, so I'll try and submit that as a separate PR that can be reviewed first.

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.

3 participants