teemuatlut / TMCStepper

MIT License
514 stars 202 forks source link

Malfunctioning 5160 commands #39

Open Psylenceo opened 5 years ago

Psylenceo commented 5 years ago

Temmuatlut thanks for the hard work on this library.

I'm working with the 5160 BOB and while testing settings to evaluate what I want to use on my 3d printer I noticed some of the commands for 5160 are either not working or just are not there. I'm not very familiar with C++ and I wasn't sure the best way to edit the code and keep it working.

The list of commands that don't work for 5160: pwm_lim(); will not compile pwm_autgrad(); will not compile pwm_reg(); will not compile pwm_ofs(); will not compile pwm_grad(); is compilable but when I read the register I get a default return value tpfd(); will not compile dc_time(); will not compile dc_sg(); will not compile

Also as a random thought without making another post, thought of doing a change log of the edits? Or am I just looking in the wrong locations?

teemuatlut commented 5 years ago

The pwm commands were very recently added in v0.4.1 https://github.com/teemuatlut/TMCStepper/commit/5762052688c52bbe47c75116d3a0d4004920ce8f#diff-be96a441b3ce400fde7703269ec9c918

tpfd, dc_time, dc_sg I think I'll take a look later once I have more time.

Each release has a list of included commits https://github.com/teemuatlut/TMCStepper/releases/tag/v0.4.1

You can also view the commit log for a more comprehensive list of changes https://github.com/teemuatlut/TMCStepper/commits/master

Psylenceo commented 5 years ago

Had a feeling I was missing something with the change log. Not too familiar with using git either><. I'll have to double check when I get home, but I'm pretty sure I had updated my library last night to 4.1 and tried to compile those commands and they were still not compiling. I'll confirm later.

As for the tpfd and dc_time and dc_sg, I read one of the other issues posts and saw that trinamics didn't think dc run was a good idea for 3d printing and the default tpfd seems to work fine just wanted to test each of these settings to see what benefits they may offer.

teemuatlut commented 5 years ago

https://github.com/teemuatlut/TMCStepper/commits/master

This now compiles

  driver.pwm_lim();
  driver.pwm_autograd();
  driver.pwm_reg();
  driver.pwm_ofs();
  driver.pwm_grad();
  driver.tpfd();
  driver.dc_time();
  driver.dc_sg();
Psylenceo commented 5 years ago

Yep, confirmed it on my end as well, except tpfd didn't I'll worry about it tomorrow. Also, I forgot diss2vs(), does not compile.

Also thanks for the updating and double checking, and the library with tmc5160 is not very friendly on SRAM. unless I have grossly done something wrong. I'm attempting to attach the code I'm using to test the individual parameters in spi mode and it uses 70% of a 2560's SRAM TMC_block_testing.zip

.

teemuatlut commented 5 years ago

You store all those debugging strings into SRAM and that's eating all of it.

Read this, especially at the end about the F() macro. https://www.arduino.cc/reference/en/language/variables/utilities/progmem/

teemuatlut commented 5 years ago

diss2vs should compile with the master branch. Let me know if that is not the case on your end.

Psylenceo commented 5 years ago

Yep, both tpfd and diss2vs compile. Apparently downloading the library through the Arduino editor missed some stuff on the 4.1 updates. So, I downloaded directly and just drag and dropped the library replacing everything and adding the new stuff.

Also, thank you for pointing out the use of progmem to save on SRAM.

Lastly, after compiling and downloading the program, I checked to see if pwm_grad and pwm_ofs pushed the value I entered and while I do get an offset read of A13 (coil a phase offset 10, coil b phase offset 19). pwm_scale_sum still shows 19. I'm wondering if either pwm_scale_sum is reading something wrong, I messed up, I'm miss understood something from the datasheet or if something else is going on. Because I thought pwm_scale_sum was to read out the value based as:

PWM_SCALE_SUM = PWM_OFS + PWM_GRAD 256 fstep / fclk

fclk = 12MHz (internal) fstep = 35.1 kHz commanding PWM_GRAD( 29) commanding PWM_OFS(65450) <- but this is commented out at the moment good thing too.

After doing the math and assuming ofs = 0, I get 21.7152.

Psylenceo commented 5 years ago

Went through and updated all my readout code, and uncommented some lines that previously didn't compile and came across 3 drive status bits that the compile says don't exist.

stealth() s2vsb() s2vsa()

And thank you for pointing me towards the flash memory function, took my code from 70%SRAM to 10%.

teemuatlut commented 5 years ago

The current updates are going to the master branch, not a release version so that's why the IDE didn't pick them up.

PWM_OFS is an 8bit value so a max of 255. Reset default for that register is 30 but as the library doesn't currently init default values for the registers (it's on the todo list), the pwm ofs would be 0 if you've pushed a value to any of the other bits in that register.

I'll add the three missing when find the time.

wesk-ms commented 5 years ago

With all the recent churn (good churn, that is) in this library and in the Marlin repo, I've started having issues with my TMC5160 drivers not being able to go into stealthchop with Marlin (not looking for help here). In troubleshooting the problem I think I found an issue that might be causing the problem @Psylenceo is seeing with PWM_SCALE_SUM.

Line 52 in TMC5160_bitfields.h defines the register address as 0x0C constexpr static uint8_t address = 0x0C;

I believe it should be 0x71 constexpr static uint8_t address = 0x71

Does that seem right @teemuatlut ? Or am I misunderstanding the datasheet?

@teemuatlut, I echo what many others have said...thanks for hard work and huge time commitment in developing and maintaining this library...AND for your contributions to Marlin!!

teemuatlut commented 5 years ago

It absolutely should. Thanks!

teemuatlut commented 5 years ago

https://github.com/teemuatlut/TMCStepper/commit/38a8fb23068957c53b9be6e9c14f3f99ca86102b

The register was moved to TMC2160 and address fixed.

Psylenceo commented 5 years ago

I was able to check last night and with the new version of the library PWM_SCALE_SUM does show better calculations. I still have not gotten to testing auto grad or auto pwm ofs yet as something I was doing between 4.1 and 4.2 or 4.3 and the latest revision no longer lets me run in silent step mode on the 5160 with spi commands. But, as soon as I comment out the silent step options I can command motions and everything is fine.

I did not feel like opening a new issue as this is more of a question than an issue and more for Marlin but since Teemuatlut adds TMC support for it I figured I ask here 1st. Is Marlin still only using SD mode for synchronization of all axis and then just using the tmc drivers spi / uart for diagnostics? Or, will it be applicable for Marlin to use the SPI / uart interface to send commands and receive diagnostics simultaneously. Just asking because I'm dreading having to make code to translate g code into spi commands to send to each tmc axis. I feel this is especially daunting when I don't even really know where I would start within the Marlin software handle g code to TMC spi commands.

teemuatlut commented 5 years ago

You'd have to compare register outputs to see what changed between the version and if it's intentional or not. The recent commits are quite small in size so stepping back in either commits or even just in releases would also narrow it down nicely.

Marlin relies heavily on the premise of controlling the drivers through S/D and changing that requires big changes. It will also never be as well supported as the more traditional mode simply because there would be so few users running the config. You'd also likely have to give up on features like S-Curve and Linear Advance. That said, if you still feel like it you can take a look at my old Trams branch from two years ago. https://github.com/teemuatlut/Marlin/tree/trams-updated

Psylenceo commented 5 years ago

I got it working again last night and tested auto-grad, auto-scale, and the auto tune for silent step. All looks good.

It looks like my issue with loosing silent step was mostly on my part. When I first got silent step working pwm_reg, pwm_lim, pwm_ofs, and pwm_grad all didn't work, so I had them commented out but the driver for some reason would let me run in silent step mode. Then we fixed those issues and I was only commanding values to start silent mode and the step threshold. That is where I believe the mistake was made at. I believe the drive expects to see pwm_lim, pwm_reg, pwm_grad, and pwm_ofs all set before the drive will actually run in silent step mode.

I knew big changes would be needed for marlin to go from SD to SPI. But, with more and more people pushing for tmc chips as drivers just to have quieter prints. I feel it's kind of a waste of pins to need 5 pins per driver, plus calculate the moves, push those moves to pulses, that constantly need updating, and also pinging the driver for status monitoring. When the use of a 5130, 5160, or 5161 would only require one additional pin per driver potentially opening up the space for more drivers, more accessory, or just more chip options. And processing the motions would (over simplified) only need to know the position to move to, accel, deccel, and velocity. And with the driver I believe running at 2Mb/s, 250KB/s, 50K registers/s, 7k accel, deccel, velocity, and xtarget commands/s, 2380 3 status register reads/s, a user could send all accel, deccel, velocity, xtarget, and read 3 status registers 595 times/s. Probably half that with the fact that we send 2 commands due to how the tmc spi is set up, my math may also be incorrect...as a matter of fact it is because i forgot to factor in the dead time that is needed in between each exchange, plus the time to toggle the chip select. But still, the data rate may be able to go higher, plus we don't always have to send all the velocity, accel, and deccel commands, especially with some diag info being sent in every data packet back to the mcu.

I agree that few users would be using a configuration like this, but that doesn't mean that more users won't adopt a system that supports this configuration or similar to it once it actually gets implemented and people can see some improvements. Be it better prints, more expand-ability, more mcu choices, faster code execution, or slimmer code ( not sure about this one).

I'm starting to look in to the s curve and linear advance stuff now, just to see if or how it could be adapted to SPI. Because my understanding of an s-curve is to do a slow accel, deccel, or velocity then ramp it up for a time, followed by a more standard shaped positioning ramp. This could in theory be performed by segmenting the s-curve so that the mcu sends a/d/v and target commands then updates them during and after the move. Since in the datasheet it states that a user can make on the fly a/d/v changes while a move is in progress but those changes won't take affect until an xtarget or xactual command is sent.

This all could just be me having high hopes, and over simplifying things that just require too much work vs using whats already tried and tested to work fine.

teemuatlut commented 5 years ago

I know it makes sense and sounds so elegant and simple, but it's tons of work and the same benefits can be had by upgrading to any number of the new 32bti boards. So because of the very low number of users this would likely benefit it's a low priority until I can address many more pressing issues and improvements first.

thalesfsp commented 5 years ago

@teemuatlut It's me again :) I'm also using the TMC5160-BOB (cc: @Psylenceo), but I'm trying to use the internal RAMP generator mode, instead of the classic DIR/STEP mode. I checked your example folder and didn't find anything like that (I'm very new to the Arduino/motion controllers/drivers/etc). I'm also not using the driver in a 3D printer, instead, I'm using that to directly control one stepper motor to close my blind :) Hope you can throw some light. Cheers!

Psylenceo commented 5 years ago

@thalesfsp in an earlier post I copied my code that has all the registers and a few serial activated commands in it. It should work for your purposes. Since it already has a basic ramp mode setup. You would just need to comment or comment registers and edit values.

Psylenceo commented 5 years ago

Version 4.5 upon running the code causes 5160 and Arduino to go into an endless reset loop that also causes the motor to pulse as if it was continuously trying to energize and then instantly de-energized.

Have not seen any issues with version 4.4 of the library.

thalesfsp commented 5 years ago

@Psylenceo I searched GitHub looking for you comment using their query language is:issue commenter:Psylenceo, but I didn't find this post. Do you mind taking a looking and sharing with me? Also, do you have Keybase chat? I see that you understand a lot about steppers/TMC, I believe you could help with my little project. Thank you!

Psylenceo commented 5 years ago

@thalesfsp my tmc block testing zip doesn't show up on the 5th comment of this thread, my 3rd comment?

If not I'm rewriting as a comprehensive example with better commenting and semi interactivity. With the program asking the user if they want adjust values and giving them the options to test the chip with out stealth mode, with out spread cycle, with stealth, with spread cycle, and be able to switch modes.

No idea what keybase even is. Knowledgable about the driver, not too sure about that. I'm just constantly looking at the datasheet and playing with the code until i see the driver do what I think it should be doing or not doing.

thalesfsp commented 5 years ago

@Psylenceo @teemuatlut Keybase is really cool, check this out: https://keybase.io/ @Psylenceo I downloaded your code, thank you very much! About the new version, do you have that somewhere? What about the wirings? Thank you guys, both, for helping :) Cheers!

Psylenceo commented 5 years ago

I dont have it complete. Actually I started making it to have messages on the terminal to guide a user. But, the complexity and time involved became too much so I scrapped it.

Then while I was cleaning up the code to be more organized with better commenting I could not get the base parameters to get the drive moving. So I've been trouble shooting that. And my supplier (buddy from work) got me 3 more drives and i got distracted with getting 3 drives to work simultaneously plus some life issues and a holiday weekend coming up is slowing me down.

thalesfsp commented 5 years ago

@Psylenceo No problem, thank you for updating back. Can you please notify me when you have your work done? Cheers!

Psylenceo commented 5 years ago

@teemuatlut stall detection during stealth chop is not possible with stallguard, however it looks like it may be possible by using the low side sense and using the driver status address, register s2vsa and / or s2vsb. The high side flag is available as s2gb and 2ga, but are only needed for short circuit detection.

I think these two flags can be added in: 5160 class of tmcstepper.h bool s2vsb(); bool s2vsa();

not sure if we would need to add a drv_status struct in the 5160 bitfield or if we can add it the 2130 bitfield.

also not sure if we need to add the below to DRV_STATUS.cpp or add it into 5160 bitfield, or make its own drv_status header. bool TMC2130Stepper::s2vsb() { GET_REG(TMC2130_n, s2vsb); } bool TMC2130Stepper::s2vsa() { GET_REG(TMC2130_n, s2vsa); }

teemuatlut commented 5 years ago

The dev branch has updated bitfields https://github.com/teemuatlut/TMCStepper/blob/9e421310dc53d9ab5236af5aa25609f2f7e09bd3/src/source/TMC2160_bitfields.h#L157-L183 But it looks like the methods needs to be added to TMC2160 as well.