hjd1964 / OnStep

Arduino telescope goto for equatorial and alt/az mounts
http://www.stellarjourney.com/index.php?r=site/equipment_onstep
GNU General Public License v3.0
452 stars 167 forks source link

MaxRate_LowerLimit missing from STM32 HAL #106

Closed kbahey closed 5 years ago

kbahey commented 5 years ago

Howard,

The latest Alpha is missing a define for the lower limit for Max Rate

I added this to make it compile:

`` // Lower limit (fastest) step rate in uS for this platform

define MaxRate_LowerLimit 16

``

But not sure if 16 is suitable with the new fractionals.

We are sure that 20 works without mode switching.

hjd1964 commented 5 years ago

I fixed the #define so that'll work but didn't change from 24us yet.

Just want to make sure since we kind of settled on 24 in this thread: https://groups.io/g/onstep/topic/28688125#6613

Fractional rates allow setting for example 22.062, 22.125, 22.187, etc. You don't have to specify them as such they'll be rounded off to 1/16ths.

The point of this is leave a fair bit of margin for error to cover the various approximations (about 1.6x faster here, about 1.7x slower there, this one 32bit that one 8bit, memory bandwidth faster slower, etc.) and options that might be enabled so OnStep isn't struggling to handle serial comms, etc during slews.

And remember even at 22 for example if you turn on pulse mode that drops to 13.75us/step which is getting near what the T3.5 could do before.

kbahey commented 5 years ago

Mihai said that he used MaxRate 20 with no microsteps slewing, and it works flawlessly.

My own testing confirms that 20 works without a hitch, also without microsteps slewing.

The 24 was to leave a safety margin.

Don't know if one is needed with the new changes anymore, or more so with them. You know.

hjd1964 commented 5 years ago

In your testing with a goto that's allowing both axes to reach full speed (covering distance in RA as well as Dec) does the workload (web) on average stay below about 50%?

kbahey commented 5 years ago

I will test again to make sure.

Something from west of the meridian low in the south, to east of it low in the south. Say Az 190 Alt 20 to Az 170 Alt 20?

kbahey commented 5 years ago

Here are the results, with latest Alpha.

With both LowerLimit (in HAL) and MaxRate (in Config) set to 20:

Slewing between above mentioned coordinates (approximately), the load is usually 30s (36%, 39%) or below, with an occasional spike to 42% or 45%.

With both LowerLimit and MaxRate set to 18%:

Same slews cause the load to reach the 60s and 70s, but no timeout.

So 20 is safe to use.

Side observations:

  1. The first access to the web page shows 502% (consistently, with no ill effect). When I select Align 1 star, the load is 156% (no ill effect).

  2. When compiling a low MaxRate (12), with LowerLimit set to 20, OnStep compiles normally. Should we have a warning saying : "You chose X but it will be no more than Y"?

hjd1964 commented 5 years ago

First access can do that, "500%", it amounts to nothing (glitch getting the first measurement.) With Align, there are delays forced due to stepper driver specs, just a blip and gone. The timing is reported as worst case since last checked so these are one off events not consistently interrupting ops.

"When compiling a low MaxRate (12), with LowerLimit set to 20, OnStep compiles normally. Should we have a warning saying : "You chose X but it will be no more than Y"?"

No, the pre-processor can't do floating point math so I abandoned that and now if you set 12 it'll be adjusted to 20us (in this case) at run-time.

Use normal sqw step mode with on-the-fly mode switching 16x to 8x (for example) and it'll be adjusted to 20*1.7=34 and 34/2 (2x bigger steps for mode switch) = 17us (slower ISRs due to mode switching code inserted combined with bigger steps make movement a little faster)

Turn on the new pulse step mode but with no mode switching and it'll be adjusted to 12.5us (20/1.6)

If the Dec axis had 2x as many StepsPerDegree OnStep will try to run that axis at twice the step rate as Axis1 so they slew at the same speed. So the limit must be raised. With everything else above considered (say no mode switching and normal sqw) you get (20us + 20us*2)/2 = 30us. It works the other way too, if Axis2 has less SPD it runs Axis2 slower, so you can overall step faster.

On top of this there's the run-time adjust-ability of 0.5x to 2x the MaxRate. OnStep will still try to allow that but if anything in that range of adjust-ability falls below the allowed rate the new rate won't be accepted.

hjd1964 commented 5 years ago

One last point that just occured to me, the run-time adjust-ability needs to be manually set to "1x" after changing MaxRate in Config.xxx.h IF REMEMBER_MAXRATE_ON is used.

OnStep will remember the rate, possibly 24us in this case, and stay there when you set MaxRate to 20us.

kbahey commented 5 years ago

Okay, so you will commit 20 to STM32's HAL?

Regarding Pulse mode, I have not experimented with it yet, but it sounds interesting, since it can improve slewing speed.

Can pulse mode be combined with microsteps slewing?

hjd1964 commented 5 years ago

Yes pulse mode has no limitations with regard to being used with micro-step mode switching.

hjd1964 commented 5 years ago

As for 20, it's really up to you. I just want to hear that REMEMBER_MAXRATE_OFF was used or you set to 1X MaxRate before testing.

kbahey commented 5 years ago

I do have it as REMEMBER_MAXRATE_OFF in my configuration.

hjd1964 commented 5 years ago

Then 20us it is.

kbahey commented 5 years ago

Regarding pulse mode, reading through your announcement of it

For the parameter name, why not something more explicit, as well as remove the HAL part (which means nothing to the end user, only relevant to developers)?

For example: instead of this #define HAL_PULSE_STEP It could be #define ENABLE_PULSE_STEP

Perhaps have it commented out in the configs with a description (I know you did not like commented out stuff in favour of _OFF, but perhaps this warrants it).

Thinking of it more, why not enable Pulse mode by default? Just because it is too new?

hjd1964 commented 5 years ago

I was thinking the same thing, it's not really a HAL parameter.

hjd1964 commented 5 years ago

I also have this bit of code I haven't done anything with yet...

// automatically set MaxRate if DefaultSlewRate is present

if !defined(MaxRate) && defined(DefaultSlewRate)

define MaxRate ((1000000.0/DefaultSlewRate)/StepsPerDegreeAxis1)

endif

// option to replace REMEMBER_MAX_RATE with REMEMBER_SLEW_RATE

if !defined(REMEMBER_MAX_RATE_ON) && defined(REMEMBER_SLEW_RATE_ON)

define REMEMBER_MAXRATE_ON

endif

hjd1964 commented 5 years ago

And this would go along with it...

define REMEMBER_SLEW_RATE_ON // Set to _ON and OnStep will remember rates set in the ASCOM driver, Android App, etc. default=_OFF.

define DefaultSlewRate 2.5 // Suggested default goto rate in degrees/second; also adjustable at run-time over a limited range.

hjd1964 commented 5 years ago

"Thinking of it more, why not enable Pulse mode by default? Just because it is too new?"

SQW mode has eased signal timing requirements. Pulse mode is probably only a problem with certain stepper drivers and some Teensy's but still defaulting to SQW mode and giving an option to change to pulse or edge mode seems prudent especially since most of the time users really don't need this kind of performance.

SQW mode has better noise immunity. Probably doesn't matter in our small case/stepper driver is 4" away from the MCU boxes but still.

The slower the processor the less the above matters and the STM32 (after the Mega2560) is the most viable candidate for defaulting to pulse mode. Put an o'scope on that step signal and get a pulse width then we can see if it'll work with the slow DRV8825 for example and get a feel for how likely it is to work with everything (all common stepper drivers vs. having another gotcha for the users.)

hjd1964 commented 5 years ago

And... How about defining a few constants and using something like:

define STEP_MODE PULSED

define STEP_MODE SQWAVE

define STEP_MODE DEDGE

kbahey commented 5 years ago

Let us make it STEP_WIDTH, so it would not be confused with mode switching (which really microstep switching).

DEDGE is so non-obvious. If it is only in the code, then that is fine, only developers will see it. If it becomes a user visible option, it will be confusing.

I am inclined to test pulsed step width directly (rather than o'scope) with the drivers I am using (two variants of LV8729), and see what happens.

Will wait until you rehash the config.

kbahey commented 5 years ago

Also, when you have time, we need a 1.16 spreadsheet with STM32 at 20.

hjd1964 commented 5 years ago

Don't like STEP_WIDTH either since it doesn't really capture the spirit of the thing. STEP_SIGNAL? STEP_SIGNAL_MODE? STEP_WAVE_FORM? STEP_FORM?

My feeling is not mentioning DEDGE is the way to go 99.999% of users won't use it. KISS.

hjd1964 commented 5 years ago

"Also, when you have time, we need a 1.16 spreadsheet with STM32 at 20."

And there's that FAQ Wiki page. And the T3.2 and T3.5 have lowered MaxRates too. And really the whole concept of the MaxRate limits have been confused with pulse mode (not that they were easily understood before!)

And the possible change to DefaultSlewRate throws the whole MaxRate thing out the window for users. Just set what speed you want and if your settings allow OnStep will try to take the motors there.

kbahey commented 5 years ago

STEP_WAVE_FORM sounds right.

Names would be: SQUARE, PULSE

Yes, let us ignore the one that will not be used.

kbahey commented 5 years ago

That slew rate thing is even better. No magic numbers that turn out to be microsconds, and no bewildered users not knowing how to tie it to degrees per second.

Perhaps call is SlewRate or SlewDegPerSec.

hjd1964 commented 5 years ago

I call it DefaultSlewRate because it is the Default since it can be adjusted from 1/2 to 2x this value at run time.

hjd1964 commented 5 years ago

Perhaps SuggestedDefaultSlewRate is even better since OnStep might decide it's too fast for the MCU.

kbahey commented 5 years ago

Maybe DesiredBaseSlewRate?

hjd1964 commented 5 years ago

That's fine with me.

kbahey commented 5 years ago

Great. Once you are done with the changes, let me know so I would test them. Also will test the wave form changes.

hjd1964 commented 5 years ago

Do you have an oscilloscope to check the STM32 pulse width?

kbahey commented 5 years ago

I do.

But it is the cheap $20 type, with low resolution and one channel. So, not easy and not a lot of confidence in that it would work.

I will just test the wave form directly with the motors to see how they do.

hjd1964 commented 5 years ago

The changes we discussed are available now in the Alpha.

It also attempts to validate the stepper driver model's pulse width spec. against what I'm guessing the various OnStep hardware produces when pulse mode is active. That is at compile-time and an #error will be thrown if the step pulse produced is too narrow for the stepper driver spec.

hjd1964 commented 5 years ago

Fingers crossed I didn't break anything, this was getting a bit complicated. :)

I issued a warning to use the Beta for now in the OnStep group.

kbahey commented 5 years ago

Well, I added DesiredBaseSlewRate and commented out MaxRate, and it compiles.

Will flash and test to see what is going on, and let you know.

kbahey commented 5 years ago

I commented out MaxRate from my Config, and set SlewRate to 5.0 (to see if it would be adjusted).

The app is showing 5.0! That is, it was not down adjusted for platform MaxRate.

Mount slews like crazy (not connected to motors) in KStars, but I am sure the motors will stall.

kbahey commented 5 years ago

I do use microsteps slewing (4) in addition to microsteps tracking (16).

No wave form parameter.

hjd1964 commented 5 years ago

:GX99# will show you the run-time lower MaxRate limit.

hjd1964 commented 5 years ago

// AXIS1/2 STEPPER DRIVER CONTROL ------------------------------------------------------------------------------------------ // Axis1: Pins (see pinmap) = Step,Dir (RA/Azm) // Axis2: Pins (see pinmap) = Step,Dir (Dec/Alt)

// Step signal wave form. Use SQUARE for best compatibility or use PULSE to allow faster step rates. Default SQUARE.

define STEP_WAVE_FORM SQUARE

hjd1964 commented 5 years ago

If STEP_WAVE_FORM is not present it'll default to SQW except for Mega2560 will default to PULSE

kbahey commented 5 years ago

:GX99 shows 20, as expected. Let is ignore wave form for now.

The problem is that the internally computed slew rate is too high. OnStep trusted my desired slew rate and made it the effective slew rate.

Now, when I clicked on the Default Speed button it went down to 1.7 deg/sec which is correct. And when I connect on Faster/Fastest/Slower/Slowest, the Current Max Rate stays at 1.7 still.

So, two issues:

a) the slewing rate in effect is the desired one, until you press Default Speed, then the correct one is in effect

b) the Faster/Slower buttons have no effect at all

I rebooted the controller and it is consistent: the Goto Speed screen shows 5.0 until a button is pressed. And even then, they go to the calculated one (1.7 deg/sec).

hjd1964 commented 5 years ago

I'm not following. What is the StepsPerDegreeAxis1? What is the DesiredBaseSlewRate?

[ MaxRate = ((1000000.0/DesiredBaseSlewRate)/StepsPerDegreeAxis1) ]

Keep in mind that unless you change a configuration setting :GX99# will always show 20. This doesn't mean 20 is being used (as a step rate) only that it's the low limit.

hjd1964 commented 5 years ago

:GX92# will show the currently set rate.

hjd1964 commented 5 years ago

If you set the slew rate at 5 deg/s why 1.7? Or is DesiredBaseSlewRate 1.7?

Almost all associated OnStep applications/add-ons have patches to work with the floating point values from the MaxRate setting/getting commands (already present in the Alpha and my ASCOM driver and the Android App these were updated in a backwards compatible way.)

hjd1964 commented 5 years ago

And as you might guess :SX92,xxx# sets the MaxRate (within the 0.5x to 2x and >=20us).

kbahey commented 5 years ago

StepsPerDegreeAxis1 and 2 are 28,800 DesiredBaseSlewRate is 5.0

The issue is that when OnStep boots. You go to the app's Goto Speed menu and it will show the desired slew rate, not the one that is computed.

When you click Faster/Slower or Default Speed in the app, then the computer one is in effect.

hjd1964 commented 5 years ago

When REMEMBER_SLEW_RATE_OFF the MaxRate wasn't being constrained. Later checks were bringing it back to the proper value on SX92.

A desired DesiredBaseSlewRate at 5.0 puts the "Desired" MaxRate at 6.94us. Even with 2x slower that's ~17us so the entire range of adjustment is outside of the STM32 capabilities.

kbahey commented 5 years ago

Exactly. The problem is that OnStep boots with the Desired as the 'in effect' and only reverts to the proper calculated one after selecting Faster/Default/Slower in the app.

Upon booting, the :GX92# says 6.938, which is too fast.

After clicking Faster in the app, GX92 returns 20, which is right.

So, it seems like the proper calculation is delayed in the code.

In Config.STM32.h, I have #define REMEMBER_MAX_RATE_OFF, but nothing about REMEMBER_SLEW_RATE_*.

hjd1964 commented 5 years ago

REMEMBER_MAX_RATE_XXX is synonymous with REMEMBER_SLEW_RATE_XXX (and another even older variation in name only all handled in Validate.h) MaxRate etc. parameters should work in backwards compatibility. This code for DesiredBaseSlewRate just calculates the MaxRate.

This issue should be fixed in the Alpha now.

kbahey commented 5 years ago

That fixed it!

Now, the issue is that Slower/Slowest/Faster/Fastest have no effect at all ... Speeds stays at the default value (1.7 deg/sec).

hjd1964 commented 5 years ago

There are two MaxRates inside OnStep. The MaxRate constant and the maxRate run-time variable. The range of run-time adjust-ability is bracketed around :GX93# return of MaxRate.

I've added code now that makes the GX93 return value >= maxRateLowerLimit().

That gets you access to "Default Speed" and slower. I'm not sure that capping :GX93# 2x slower so we cover the full adjust-ability range is the way to go, or not.

On one hand full adjustability is nice on the other you easily get into a situation where the user asks for say 2 deg/s and gets 1 (until they adjust it at run-time) where the reasonable expectation is for things to come up at the DesiredBaseSlewRate if that is possible.