tmc2240: add current formatters

kdomanski commented 7 months ago

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:

  // DRV_CONF: 00000001 current_range=1 (1.41A RMS)
  // GLOBALSCALER: 0000007f globalscaler=127
  // IHOLD_IRUN: 04061f1f ihold=31 (0.70A RMS) irun=31 (0.70A RMS) iholddelay=6 irundelay=4

github-actions[bot] commented 6 months ago

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:

Perform a self-review of your Pull Request by following the steps at: https://www.klipper3d.org/CONTRIBUTING.html#what-to-expect-in-a-review If you have completed a self-review, be sure to state the results of that self-review explicitly in the Pull Request comments. A reviewer is more likely to participate if the bulk of a review has already been completed.
Consider opening a topic on the Klipper Discourse server to discuss this work. The Discourse server is a good place to discuss development ideas and to engage users interested in testing. Reviewers are more likely to prioritize Pull Requests with an active community of users.
Consider helping out reviewers by reviewing other Klipper Pull Requests. Taking the time to perform a careful and detailed review of others work is appreciated. Regular contributors are more likely to prioritize the contributions of other regular contributors.

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, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

kdomanski commented 6 months ago

Self-review checklist

1. Is the submission free of defects and is it ready to be widely deployed?

Yes, the submission passes regression tests
it has been tested manually as well
it doesn't add any logging or debugging code

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.

KevinOConnor commented 6 months ago

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 SET_TMC_CURRENT command (without arguments).

-Kevin

KevinOConnor commented 6 months ago

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, -Kevin

kdomanski commented 5 months ago

Hey Kevin, it definitely was a mistake on my part to miss def get_current(self) which was right there. -,-' I'm happy to rework and simplify this, if you'd like.

I fear it could be confusing to users that a dump of the IHOLD_IRUN register reports different descriptions for the same register content.

By "different descriptions", do you mean both peak-to-peak and RMS? I can remove the first one, if you'd like.

It also makes the tmc2240 dump output different from all the other tmc drivers.

Is that really an issue? The output is already different, because drivers are all different and have different registers. I know that you mean the specific part regarding current, however it sounds a bit like "either we implement this for all TMC drivers or none of them". Is that the idea? If so, I might need someone to collaborate with on this one, because I simply don't own every TMC driver supported. Right now, I could extend this to TMC2209, which I also have.

Or perhaps the documentation could make it more clear that the information is available from SET_TMC_CURRENT.

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 SET_TMC_CURRENT command will not result in more people reading it and being aware. Maybe you or @Arksine have a better idea though.

KevinOConnor commented 5 months ago

By "different descriptions", do you mean both peak-to-peak and RMS?

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

github-actions[bot] commented 5 months ago

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, ~ Your friendly GitIssueBot

PS: I'm just an automated script, not a human being.

Klipper3d / klipper