teemuatlut / TMCStepper

MIT License
514 stars 202 forks source link

ENC_CONST() should be 32bits but is coded as 16 bits #80

Open otispdawson opened 5 years ago

otispdawson commented 5 years ago

I'm an electrical engineer (not software super person) using a TMC5160 with an encoder. I found that no matter how I set ENC_CONST using this library's function, it would only ever set the lower 16 bits (called the FRACTION in the datasheet) and never set the upper 16 bits (called the FACTOR). After poking around the code, I found the code accepts uint16_t type for ENC_CONST. Here and elsewhere: https://github.com/teemuatlut/TMCStepper/blob/2d14f340a0966a16ac66ecfa435434aac3dd0f7d/src/source/TMC5130Stepper.cpp#L214

However, the TMC5160 datasheet says that ENC_CONST as 32 bits wide. image So this explains how it was always setting just the lower 16 bits because it was casting away the upper 16 bits of my 32 bit command word. I fixed it on my machine by changing the all references to ENC_CONST() to accept and return uint32_t instead of uint16_t. However, I'm not comfortable enough with the overall library code to trust my fixes enough to submit a pull request. That is, maybe the TMC5130 needs 16 bits and only the TMC5160 needs 32 bits or something, I don't want to break anything.

Here's my hacky fixes (change uint16_t to uint32_t): TMC5130Stepper: // W: ENC_CONST uint32_t TMC5130Stepper::ENC_CONST() { return ENC_CONST_register.sr; } void TMC5130Stepper::ENC_CONST(uint32_t input) { ENC_CONST_register.sr = input; write(ENC_CONST_register.address, ENC_CONST_register.sr); } TMC5130_Bitfields: struct ENC_CONST_t { constexpr static uint8_t address = 0x3A; uint32_t sr; }; This library is incredibly useful! Thanks for all the hard work!

teemuatlut commented 5 years ago

Fixed https://github.com/teemuatlut/TMCStepper/commit/b5b3658d34f93e0cc646dbaa54f90b809267206f

I'm also considering adding methods for the two parts but unsure of the types. For now it can be as a 32bit "binary blob", but if I add the distinct parts should the integer part be int16_t and the franctional uint16_t