Add Chain Wrap Direction #382
Conversation
This adds a configuration setting to select which way the chain wraps around the sprocket to go to the motor. Changes are also made to update the triangular kinematics calibration code. When a new value of chainOverSprocket is received the axes are reconfigured.
|
Do you guys think this requires incrementing the firmware version tracking number? |
|
Does there need to be some way for the firmware and GC to signal compatibility on this? Or does GC even care? Will GC need different calibration routines for this arrangement? |
|
GC doesn't really care. In either case, GC will send the same code to the firmware. Really, the firmware needs to know which way it should spin the motors to feed out positive chain. Tweaks to the calibration process in GC were made. The first was to match the new firmware triangular kinematics. The second was to update the motor spacing measurement process to allow the "bottom" configuration. The default value in GC and Firmware is the "top", which is what the traditional design uses. So today a mismatched GC and Firmware shouldn't create issues. |
| //Calculate the chain angles from horizontal | ||
| float Chain1Angle = 3.14159 - acos(R/Motor1Distance) - acos((_yCordOfMotor - yTarget)/Motor1Distance); | ||
| float Chain2Angle = 3.14159 - acos(R/Motor2Distance) - acos((_yCordOfMotor - yTarget)/Motor2Distance); | ||
| //Calculate the chain angles from horizontal, based on if the chain connects to the sled from the top or bottom of the sprocket |
There was a problem hiding this comment.
This is so cleanly done. Excellent! I was worried this would require all types of changes throughout the code 👍 👍
cnc_ctrl_v1/Kinematics.cpp
Outdated
| float Chain2Angle = 3.14159 - acos(R/Motor2Distance) - acos((_yCordOfMotor - yTarget)/Motor2Distance); | ||
| //Calculate the chain angles from horizontal, based on if the chain connects to the sled from the top or bottom of the sprocket | ||
| if(sysSettings.chainOverSprocket == 1){ | ||
| float Chain1Angle = asin((_yCordOfMotor - yTarget)/Motor1Distance) + asin(R/Motor1Distance); |
There was a problem hiding this comment.
Unfortunately I don't think C++ lets us create the float variable within the if statement (python would be OK with it, but C++ is picky)
I think we need to define float Chain1Angle = 0; float Chain2Angle = 0 just above the if
There was a problem hiding this comment.
Thanks for the catch! I'll fix that now.
Define the variables outside the if statements
|
|
||
| //Set up variables | ||
| float Chain1Angle = 0; | ||
| float Chain2Angle = 0; |
|
There is something fishy going on and I think it's in the kinematics changes. I'm getting this warning every time I connect, and the motors refuse to move even if I do a manual chain length reset:
Does anyone have any thoughts on where to look? |
|
Is that with the chains on bottom of the sprockets? Does it still happen with them on top? |
|
Yes, changing the setting doesn't seem to matter which is strange. When I print the value for the length of chain that the kinematics is recommending right before it is returned from the kinematics function I see very reasonable values
|
|
What values for the chain lengths does the kinematics get to before failing? I'll review it too. |
|
Is it possible that running setupAxes() when pushing the settings from GC to the firmware is causing the issue? I'm wondering if setupAxes() works properly after the system is initialized. Specifically when detecting the PCB version and assigning pins, there is no catch-all. So if the PCB detection didn't work, then the system would lose the ability to communicate with the motors. |
|
It looks to me like it isn't trying to guess anything other than zero for the coordinates in the forward kinematics. It just keeps trying zero over and over until it times out. |
|
That would imply that aChainError and bChainError are 0, but if that were the case then it would have exited successfully. Strange. Could you try modifying your version of the test code, commenting out line 390 of Settings.cpp (this is where it calls setupAxes()). Then try again. |
|
I see, upon looking closer it is getting the chain lengths reported from the motors as being 0 for both. There is no position which would satisfy that, for the sprocket position on top or bottom, and hence it fails out. So something is interrupting the encoder from reporting the chain position, or it got lost somehow. |
|
Commenting out that line did not change anything, but I confirmed that the kinematics are in fact being told that the chain lengths are zero. So why doesn't doing a manual chain length calibration fix it? The manual recalibration should reset them to a known length, right? |
|
Because I am thinking that the pin assignments to the motors got overwritten. The motors have never moved, right? Does the Z axis move? And have you restarted your Arduino? |
|
I'm wondering if the proper way to do this is to call the systemReset() function, rather than setupAxes() directly. This would detach the axes and repeat the power-up process, which is probably the better way to do this. But if it still doesn't work after restarting your Arduino with that line commented out then something else is wrong. |
|
@reecej you are a genius! First I checked to see if the z-axis would move and it did. 🎉 Then I powercycled the Arduino, re-did the manual chain length calibration to set the chains to a good length, and the motors spin 👍 👍 💯 🎉 |
|
To confirm, that was with that one line still commented out, correct? If so, then your motors aren't yet being reversed for the chain on bottom configuration. Does anyone know if anything breaks if we call systemReset() as a method to re-configure the pins? I would imagine we should do some error-checking to ensure this is only called if the value actually changes. Ideally, since we have already determined the PCB version, this seems like a lot of work to do, but it may be the cleanest approach. |
|
This was with the line commented out. I believe (but I am not 100% sure) that I had tried re-starting the arduino with the line in place with no luck. I agree that we should only call a systemReset() if the value changes. If we called it every time I could imagine it causing the connection to close and open making GC send the values again, causing a loop |
Call systemReset if the chainOverSprocket value changes, and do nothing otherwise.
|
With this last commit, it will call systemReset only if the value changes. Would calling systemReset cause it to lose connection to GroundControl? |
|
I will test it and find out! |
|
I appreciate your help, as always! Soon I'll be able to do this testing myself! |
|
Alright! We're making good progress! I had to add the systemReset() function to system.h to make it publically available, but then everything fired up!
I'm going to start putting it through its paces |
|
Thanks for the info! I'll push that in a commit to get that sorted out. |
Make systemReset public.
|
When the chain is set to bottom all is well 🎉, both motors move, the position is saved between power offs, and when the machine connects everything is normal other than that the position seems to be loaded twice:
When I switch it to the Top setting I see some strange behavior. The motors won't move and the machine clearly goes through the reset cycle even if this isn't the first time the GC has been launched after the setting was changed.
Because this setting is very startup specific, and because it's not something people will be changing often, what if we just required a power cycle for it to take effect? Like instead of triggering a reset when it is changed we just stored the choice in EEPROM and used it at startup. If you send a new setting to the board it gets written to EEPROM with all the rest of the settings but won't be active until the board is power cycled. It's not an ideal solution, but it seems like a good place to start. |
|
Good point. I wonder if the system reset happens too quickly, specifically before the value gets written to EEPROM. In that case every time it connected to GC and pulled down the settings, it would just reset again. Storing it to EEPROM and then having users reset the board I would think would be a fine initial solution. Later on I suspect I could figure out the structure and change the settings without requiring all this work, but given how rarely this option will be changed I think requiring a restart is fine! Should we include this in the calibration steps? |
Store new value to EEPROM and wait for user to reset system to take effect.
|
It's been my experience that the things which happen at startup can be a little touchy. I like this plan as a starting point. I vote we leave it out of the calibration until we get this PR merged and then we either find a way to make the reset automatic or mention it in the calibration in the next PR mostly because I feel like there has got to be a better way to use git than to have me keep sending you screenshots of lines of code to change...but here's one last one 😜 I think these two lines in cnc_cntrl_v1 should be swapped, right? That way the setting will be pulled from the EEPROM before it is needed? |
|
That's true! I wonder if it would be easier to give you rights to my fork, that way we can use Git more for collaboration rather than sending screenshots. And good point on the settings as well as GroundControl! The only caveat with GC is the step where the motor spacing is measured using the chain. But I have an idea to make that work. |
Load settings before setting up axes.
|
I have an idea to make that work too 😀 I think this side of things is ready to merge! I just went through the process of switching over and doing the extend chains, attaching the sled and moving everything around and it all looks good! |
|
My idea isn't very smooth - basically, if the chains are on bottom of the sprockets then just send reverse distances for whatever the user requests, and keep the chain on top for that portion of the calibration. But that discussion can be continued over there! |
|
I'm going to merge this and move to thinking about that! |
|
Thanks for sticking with me through all that testing. It's a tedious process to do things this way and it takes patience for sure. I can wait until you have hardware to test on, but I really appreciate all the work you are doing already! |
|
@BarbourSmith , what did you do to get past this? I've got the current master firmware and the PR version of GC. I've been through the triangular calibration upside down twice, good results both times, and can run gcode files, but after quitting GC and opening it again, I need to re-cal the chains every time. I've tried an EEPROM reset, and a fresh groundcontrol.ini file. I can see that the EEPROM is being read and passing verification. |
|
Does that only happen for the chain on bottom case, or does it also prompt you for that with the chain on top too? |
|
Yes, with the chain on top too. |
|
I'm wondering if that is related to us changing the order and having the settings be loaded before the axes are configured. We need the settings to know how to configure the motor pins, per the chain direction, but the settings also load the last motor position. However, it looks like the motor positions then possibly get overwritten when the axes are configured. This would make it such that every boot up the chain lengths are thought to be zero, which seems to be what you're seeing. @BarbourSmith, did you experience this also? |
|
@blurfl could you give my code a try? I just tried a patch which should load the encoder steps after the axes are setup. https://github.com/reecej/Firmware, master branch. |
I see the same behavior you are seeing but only right after I switch from the top setting to the bottom setting or back. Every time I switch I have to reset the chain lengths (either manually or with the autocal), but once I've done that I'm seeing everything stay good through multiple GC restarts. Unless I change the setting again things are OK. My thinking was that because we're essentially reversing the direction of the axis the old number is invalid, but I want to dig into it more and really understand what is happening. |
|
@reecej , your code works correctly for me. Using then PR version of GC and your code, the dialog came up the first time I connected, but after the manual chain cal/return to center boogie and quitting GC the dialog no longer comes up. |
|
@blurfl to confirm, your last test was done using the release version of firmware code, or the current master version? I suspect the version we merged from this PR into the master accidentally clears the stored last chain length position, hence the message. But ideally if you were running the latest released version of firmware and then moved to this code, assuming you kept the chain on top, I wouldn't think there should have been an issue. |
|
I was using the current master version. I'm willing to set up any configuration you'd like. |
|
I wonder if that's because the settings structures are different among both. If you move straight from 1.04 releases for GC and Firmware to the GC PR and the firmware in my fork, do you get the same thing? From how it looks in the code, this may be the expected behavior, but I'm not familiar enough with the code to say for certain. Regardless, it sounds like it isn't happening every time like it was before for you? |
|
It was happening every time that I was using any combination other than Fv1.04/GCv1.04, sorry if that wasn't clear. |
|
Cool, that sounds promising. Seems like I should take the patch written in the firmware and submit that as a PR then? |
|
I'd say yes! |
|
Fix submitted in PR 383! |
This adds a configuration setting to select which way the chain wraps around the sprocket to go to the motor. Changes are also made to update the triangular kinematics code. When a new value of chainOverSprocket is received the axes are reconfigured.
Note that this does not update the quadrilateral kinematics, so using quadrilateral kinematics with the chains on bottom of the motor sprockets could lead to accuracy errors.
When a new value of chainOverSprocket is received, setupAxes() is called. I don't think this would cause issues, but wanted to point it out for review.
An associated GroundControl PR will be submitted shortly.