[LinuxCNC/linuxcnc PR#525] Apply ‘max velocity’ to pure rotary motion

#1 – zultron 于 2018-11-06

This won’t work right out of the box:

– Configurations need to add a MAXANGULARVELOCITY parameter in the [TRAJ] section
– UIs need to read the parameter when reading MAX_VELOCITY
– In my case, the UI treated the max velocity slider as a percentage, and used the setting to scale both linear and angular velocity before calling command.maxvel(scaledvallin, scaledvalang)

#2 – zultron 于 2018-11-06

I see the changes to the Axis GUI actually are in place. However, they’re probably untested, and other GUIs haven’t been updated.

#3 – c-morley 于 2018-11-06

I tested John’s fix in simulation and it indeed limits angular only movement – Yaay!
Thanks for addressing this John.
The units on the slider of course, make no sense for angular movement.

reading John’s comments here:

The .ini docs place MAXLINEARVELOCITY and
MAXANGULARVELOCITY in the [DISPLAY] section; the former is the
maximum possible value on the max velocity slider. The MAX_VELOCITY
param is in the [TRAJ] section, and is the set value on the slider.
This naming makes it hard to add the missing angular equivalent to
[TRAJ]MAX_VELOCITY

I looked at the docs for master and those terms in the TRAJ are different.
eg. MAXLINEARVELOCITY -for master
I haven’t dug into the code to see what actually uses these entries and how they should be spelled- I wonder if the INI docs lie…. I know in GUI building the TRAJ and DISPLAY terms were often confused.

For the GUI I wonder why you would limit the slider at all – it should be the machine’s max possible velocity – which I assume would be from the TRAJ or JOINT sections.

overall thought (not all are directly about the patch):

This is a excellent improvement.
Should we put this in 2.7 ?- while it is a bug fix, it touches a lot of code.
There is question what the slider units should display
If not %, there is question where the GUI should read it’s max setting from
I wonder why we need this capability anymore – overrides seem to do it better.

#4 – zultron 于 2018-11-09

Thanks for the thoughtful comments, @c-morley , and the encouragement, too. I’ll need to find time to think about your .ini questions; I remember during implementation it was difficult to figure out in which sections the various parameters belonged.

I’ll also have to find time to update the other GUIs besides Axis. I’m hoping someone will jump in to help out there; it should be easy for someone with prior experience.

#5 – andypugh 于 2018-11-09

On Fri, 9 Nov 2018 at 04:01, John Morris wrote:

> Thanks for the thoughtful comments, @c-morley
> <https://github.com/c-morley> , and the encouragement, too. I’ll need to
> find time to think about your .ini questions; I remember during
> implementation it was difficult to figure out in which sections the various
> parameters belonged.
>

2.8 provides as opportunity to move things around in the INI file. We are
already introducing significant INI file layout changes for JA, and there
is a script to handle the reformatting.
I think that it an INI file entry should move, this is the time to do it.

—
atp

#6 – zultron 于 2018-11-09

> 2.8 provides as opportunity to move things around in the INI file. We are already introducing significant INI file layout changes for JA, and there is a script to handle the reformatting. I think that it an INI file entry should move, this is the time to do it.

Good point. This patch doesn’t change existing INI params, but adds one new one. Like the MAX_VELOCITY param, it defaults to 0.1 * googol, so its absence won’t break existing configs.

#7 – c-morley 于 2018-11-10

I can help with Gmoccapy and Gscreen GUI changes – but I’m fairly busy for the next month.
Is this something we are trying to get into 2.8 ? I heard a tentative date of end of DEC….

#8 – gmoccapy 于 2018-11-11

I can add that to gmoccapy, just mention witch branch I need to test?

Norbert

Am 10.11.2018 um 05:32 schrieb c-morley:
>
> I can help with Gmoccapy and Gscreen GUI changes – but I’m fairly busy
> for the next month.
> Is this something we are trying to get into 2.8 ? I heard a tentative
> date of end of DEC….
>
> —
> You are receiving this because your review was requested.
> Reply to this email directly, view it on GitHub
> <https://github.com/LinuxCNC/linuxcnc/pull/525#issuecomment-437557542>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AGgE-eszVUMQoqththwkguM3Ep8uLvcoks5utlb5gaJpZM4YPwHL>.
>

#9 – KurtJacobson 于 2018-11-11

@zultron This looks great! Thank you.

One comment on the python interface changes: I see that you added a second argument to command.maxvel for setting the angular max velocity. I would prefer to see separate command methods for setting the max linear velocity and max angular velocity.

There are several advantages, one being that UIs that currently use command.maxvel would not be effected, and would continue to work as before.

Second, it is most likely the max vel values will be set by UI sliders or the equivalent, meaning that the linear vel and max angular vel will be set independently. The new command.maxvel method with two arguments would require the UI programmer to first get the value of the override he did not want to change, resulting in something like this pseudo code:

“python
def onmavsliderchanged(newmav):
"""Set max angular velocity."""
stat.poll()
currentmlv = stat.maxvelocity
command.maxvel(currentmlv, newmav)
“

I also think having separate command methods is more consistent with the rest of the changes.

#10 – andypugh 于 2019-04-28

Where are we with this one? Do we want to introduce a new INI item inside the conversion script?

#11 – zultron 于 2019-04-29

I haven’t worked on this one for a while, but it’s not ready. I think @KurtJacobson is right, that the commands should be separated so that the GUIs don’t have to be modified.

What’s a conversion script, for updating user configurations? That could work: Add [TRAJ]MAXANGULARVELOCITY, copying the value of [TRAJ]MAXLINEARVELOCITY.

Another way to handle the transition is to make this a feature that must be turned on; by default, max angular velocity would simply track max linear velocity, as it does right now. Then users who care about this can add it to their configurations.

#12 – andypugh 于 2019-04-29

Because of the JointsAxes and multispindle changes that are in master / 2.8 there is a script that automatically edits the INI and HAL files to deal with the new HAL pin names and config. It would be easy to make this also add a new MAXANGULAR_VELOCITY.
https://github.com/LinuxCNC/linuxcnc/blob/master/scripts/update_ini#L227

#13 – petterreinholdtsen 于 2022-08-14

Is there any hope to brush up this pull request to a point where it can be applied to master? Is it complete now, or is more work needed before it can be merged into master?

#14 – andypugh 于 2023-06-18

Discussing this at the Norway meeting, we think that this is now too old to merge.
We like the idea, but this PR isn’t mergable.