gin66 / FastAccelStepper

A high speed stepper library for Atmega 168/328p (nano), Atmega32u4, Atmega 2560, ESP32, ESP32S2, ESP32S3, ESP32C3, ESP32C6 and Atmel SAM Due
MIT License
301 stars 70 forks source link

Due/SAM influence of other pwm pins? #140

Closed sigmaeo closed 1 year ago

sigmaeo commented 2 years ago

When I uses moveTo(1000, true) (=with blocking) the function moves the motor (with step-pin connected to PWM7) as expected, but it sets PWM6 at the same time from HIGH to LOW. When I use it without blocking moveTo(1000, false); this doesn't happened.

cu joern

gin66 commented 2 years ago

Hi,

the sam/due port is developed by @clazarowitz. Perhaps he can have a look.

From studying the code, there may be a side effect of disconnecting a pin. Could you please locally patch StepperISR_due.cpp and try again ?

Please replace line 252:

PWM_INTERFACE->PWM_DIS = mask;

with

PWM_INTERFACE->PWM_DIS |= mask;

and line 309

  PWM_INTERFACE->PWM_DIS = channelMask;

with

  PWM_INTERFACE->PWM_DIS |= channelMask;
sigmaeo commented 2 years ago

Hi, thank you for your solution approach, but I see no difference with these patches.

After many tests I now know a little bit more about my problem: My influenced pin, that is always set to LOW by a moveTo() call, is another PWM pin and this undesirable side effect "works" with different PWM pins (6,7,8,9,...?). But it works only, if I do sometime before a:
analogWrite( mypin, 65535 ); (Furthermore i only set mypin as output and to HIGH.) All PWM-pins that I only set to HIGH remain HIGH, but the ones, that I set with analogWrite() sometime before become LOW after moveTo(). If I call after the moveTo() again:
analogWrite( mypin, 32000); nothing changes, mypin remains at 0V.

I need 2 lines to reactivate my PWM-output: digitalWrite( mypin, HIGH ); analogWrite( mypin, 32000 ); or pinMode(mypin, OUTPUT); analogWrite( mypin, 32000 ); Moreover: mypin already goes low while the engine is still running, i.e. the problem seems right at the beginning of moveto().

cu joern

clazarowitz commented 2 years ago

Sorry I'm a bit slower to respond....I have a LOT going on :) In fact, I was about to dive back into another project, but remembered this! Waiting probably helped, as I think I know whats going on. I'll need to go through the data sheet and Due pin mapping, but I think this is...sort-of, expected....and should have been documented.

AnalogWrite uses the PWM peripheral. So any other PWM peripheral on the same device is going to be "reprogrammed". IIRC there are multiple channels in the PWM peripheral. analogWrite expects the peripheral to be setup by the arduino code with certain clocks, and in certain ways to do "arduino" "analog" "things". Stepper control is NOT one of them ;) I change the programming of the peripheral to be able to generate the pulse ramps.

As I said, let me dig back into the datasheet, and see if there is something that can be done. At a minimum, I know several of the PWM pins are actually timer controlled rather than PWM controlled, and I'm fairly certain I stayed away from those pins when initializing my timers for the control cycle of the ramp generator. I should respond back tonight....maybe quickly if my memory returns quickly!

clazarowitz commented 2 years ago

Ok, for sure, pins 2, 3, 4, 5, 10, 11, 12, and 13 should be completely unaffected...(I'll double check my timer code though) those are done on the timer hardware instead of the PWM generator. that can be found in the Due support library in variant.cpp on my windows computer at %AppData%\Local\Arduino15\packages\arduino\hardware\sam\1.6.12\variants\arduino_due_x\variant.cpp and notice they're pwm pins and have PIN_ATTR_TIMER in their attributes rather than PIN_ATTR_PWM

I suspect 6, 7, 8, and 9 will not be usable for analogWrite, but I'll double check that as well....and I'll make sure it gets in the documentation :)

clazarowitz commented 2 years ago

Ok....I see an issue...PWM_DIS is write only (duh) yet I'm performing an & operation to disable....Which is why you don't want to use the code changes recommended above :) Writing 0 has no effect, so you want to only write the bits you want to influence.

Instead, try changing every instance of PWM_INTERFACE->PWM_DIS = PWM_INTERFACE->PWM_DIS & (~mapping->channelMask); to PWM_INTERFACE->PWM_DIS = mapping->channelMask; I'm not sure what the read of PWM_DIS will contain....so its possible I'm disabling all channels in those functions...oops. Should be lines 542, 544, 546, and 586. Seems I learned my lesson elsewhere in the code, but missed these as they aren't part of the dance of pausing. :) I also....think....I used clock B for my pulses, so analogWrite may actually still work as expected. Or it may break the stepper code. I'll have to look into it.

If that doesn't fix it, let me know and I'll try to reconstitute my testbed and reproduce. Otherwise, I'm not sure why other pins would be influenced.

It will be a while though (easier if you can report!) I'll be on vacation this coming week for a while, and almost always after taking time off, things that need to be done pile up. On top of all of that, I may move out of my current house to a new place across the country. I have a pretty big CNC machine shop that will also need to be moved, and a new shop built! So I have a lot going on. It would be far easier to not move, but I don't know if that will be possible!

sigmaeo commented 2 years ago

Thanks for your help. I changed these lines, but the result is the same.

If the pwm on "mypin" would stop, it wouldn't be so bad for me. What is annoying is that the pin goes LOW - even High would not be so bad for me. If I hadn't used analogWrite(mypin) with before, then the HIGH would just stay and not change. So as a minimal solution, I would have to undo analogWrite(), so to speak.

Unfortunately, I am not familiar with the registers of the SAM and your comments in the source code make me afraid to look at the (obviously terrible) data sheet. ;-)

If I switch "mypin" to INPUT_PULLUP before calling moveTo(), the voltage on this pin drops from my PWM-voltage (for example 2.5V or 3.0V) to 1,8V (instead to 0V as before) and if I switch mypin after moveTo() back to an output, the voltage rises always to 3.3V. Sorry I currently have no oscilloscope to check if the 1.8V is due an (unwanted) PWM on mypin.

P.S. You wrote "PWM_DIS is read only", but you meant "PWM_DIS is write only".

clazarowitz commented 2 years ago

Good catch…yeah, no one wants more confusion! That data sheet is confusing enough as it is! ;)

I will see if I can get things setup tomorrow.

I did something kind of…probably…stupid today instead. I gambled on buying a used polyjet 3d printer from the local university surplus and salvage department! It weighs about 550 lbs (somewhere around 254 kg iirc). I left work at 1:00pm to pick it up. I got it in the house at 9:00pm. Obviously I had to at least power it on! I ran the self tests and so far so good. Unfortunately, I need $800 in resin to test the print heads, so that will wait a while. I got the printer, an objet Eden 260 for $560, so if it works, it’s an amazing deal. But I was pretty tired after all that, so I didn’t get my due/stepper test bed set back up. Sorry about that!

one other thing to try, if it being high is ok, are you using pwm on the pin? Setting it to digital and using digitalWrite will disconnect it from the pwm peripheral and instead to the pio controller, so it should be unaffected by anything pwm. I would have run this test if I had everything setup; as it will help to pinpoint the issue. Obviously, it shouldn’t be messing with other pins if it is at all possible, and since I’m using pins 6 and 7 in my application, it should be possible. I just suspect some shenanigans from the arduino analogWrite command. I know it has some initialization code (if it thinks the pwm peripheral isn’t initialized) and it may have some enforcement code of registers. I can’t remember. Taking it out of the loop would be a useful test though!

clazarowitz commented 2 years ago

Ok, I committed in place...poor form, but I'm going to go to bed. I'm exhausted after yesterday, and had planned on being asleep 2 hours ago!

So technically the commit is untested, but I copied and pasted from my working copy.

Basically, I had worked in code to check for PWM events from outside the stepper code (like analogWrite) but never put it in the PWM handler. So it was just a matter of looking to see if the event came from one of the channels we are controlling, and if not, continue on and ignore the event. I had actually thought that the interrupts wouldn't have been enabled by default!

I also found and described in the commit message that changing the B clock while something was using the A clock could cause the channel to stop entirely. However, I also realized MCK/2, MCK/4....up to MCK/1024 are all provided without using clock channel B which is actually divider B, for a custom frequency B. As I was setting B to MCK/4, it was pointless to setup Clock B, so we could leave analog writes alone. Just took a small change to the ConfigureChannel call to point at the built in MCK/4 instead of CLKB.

That should fix it. If I don't hear anything by tomorrow night my time, I'll backup my copy, and test it with my oscilloscope. I didn't setup a stepper motor....just the scope....

sigmaeo commented 2 years ago

Hello,

fantastic, your changes helped - now the analogwrite()-PWM on mypin is no longer influenced by a moveTo(). :-)

I thought I had already tried "setting it to digital and using digitalWrite" with mypin, but apparently I still had an analogwrite behind it, which is why mypin never stayed HIGH while moveTo(). But now I tested it and setting it as a normal output actually helps as an alternative solution, but that is no longer necessary with your fixes.

Many thanks for your quick help!

Cool 3D-printer :-) It's actually impossible to get your hands on something this cheap if it still works.

sigmaeo commented 2 years ago

Before closing this issue it would be good to change the lines too: PWM_INTERFACE->PWM_DIS = PWM_INTERFACE->PWM_DIS & (~mapping->channelMask); Because the "PWM_DIS is write-only"-issue.

clazarowitz commented 2 years ago

There are actually other instances of reading a write-only register beyond that one. Those are some of the changes in my local copy that I didn’t want to lose. I also want to audit all of the register operations to make sure I’m not missing any.

I had a few other small tweaks as well. I might not get to fixing all of that before going on vacation though. So I wanted to get a working solution before I disappear for a week and a half :)

gin66 commented 1 year ago

stalled