-
-
Notifications
You must be signed in to change notification settings - Fork 5.4k
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
tmc2240: add current formatters #6503
Conversation
Run current, hold current and current range will be shown in Amperes. Each 2240 driver now has a separate instance of FieldFormatters, since the current formatters need to access a few fields in order to calculate the current. Signed-off-by: Kamil Domański <[email protected]>
dde2c7c
to
6086ca1
Compare
Thank you for your contribution to Klipper. Unfortunately, a reviewer has not assigned themselves to this GitHub Pull Request. All Pull Requests are reviewed before merging, and a reviewer will need to volunteer. Further information is available at: https://www.klipper3d.org/CONTRIBUTING.html There are some steps that you can take now:
Unfortunately, if a reviewer does not assign themselves to this GitHub Pull Request then it will be automatically closed. If this happens, then it is a good idea to move further discussion to the Klipper Discourse server. Reviewers can reach out on that forum to let you know if they are interested and when they are available. Best regards, PS: I'm just an automated script, not a human being. |
Self-review checklist1. Is the submission free of defects and is it ready to be widely deployed?
2. Does the submission provide a "high impact" benefit to real-world users performing real-world tasks?Yes, it prevents the necessity to manually calculate current values when debugging the behavior of TMC2240 drivers. 3. Is the copyright of the submission clear, non-gratuitous, and compatible?Yes. 4. Does the submission follow guidelines specified in the Klipper documentation?Yes. 5. Is the Klipper documentation updated to reflect new changes?The completeness of field formatters is not documented nor does it need to be. Thus adding a few formatters requires no documentation change. 6. Are commits well formed, address a single topic per commit, and independent?Yes. |
Thanks. I'm not sure about this change, as there isn't a simple mapping between ihold/irun and amps. I fear it could be confusing to users that a dump of the IHOLD_IRUN register reports different descriptions for the same register content. It also makes the tmc2240 dump output different from all the other tmc drivers. A user can obtain the power settings via running the -Kevin |
For what it is worth, perhaps an alternative would be for DUMP_TMC to report the power settings (as SET_TMC_CURRENT already does). Or perhaps the documentation could make it more clear that the information is available from SET_TMC_CURRENT. Cheers, |
Hey Kevin,
By "different descriptions", do you mean both peak-to-peak and RMS? I can remove the first one, if you'd like.
Is that really an issue? The output is already different, because drivers are all different and have different registers.
That would definitely help, however I cannot find a good spot to do it. I'm afraid that simply adding it in the reference docs for the |
I mean, for example, that the IRUN field may have a value of 5, but that may translate to a description of 0.6amp, 1amp, 2amp, etc. That is, it may be confusing to describe the IRUN field using settings that are external to the IRUN field. -Kevin |
It looks like this GitHub Pull Request has become inactive. If there are any further updates, you can add a comment here or open a new ticket. Best regards, PS: I'm just an automated script, not a human being. |
Run current, hold current and current range will be shown in Amperes. Each 2240 driver now has a separate instance of FieldFormatters, since the current formatters need to access a few fields in order to calculate the current.
Output sample: