Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added simple test that ensures accelerations constraints are met. #44

Merged
merged 11 commits into from
Apr 7, 2022

Conversation

lromor
Copy link
Contributor

@lromor lromor commented May 3, 2021

No description provided.

@lromor lromor marked this pull request as draft May 3, 2021 22:58
@lromor lromor force-pushed the lookahead branch 2 times, most recently from ae91880 to 98b8b89 Compare August 22, 2021 20:22
src/planner_test.cc Outdated Show resolved Hide resolved
@lromor lromor force-pushed the lookahead branch 2 times, most recently from b90fd7b to 845756f Compare September 12, 2021 12:50
@hzeller
Copy link
Owner

hzeller commented Jan 31, 2022

Looks like there is a conflict in one of the files; the last change in the file was in #47 - maybe things got messed up in working in two branches ?

src/planner.cc Outdated Show resolved Hide resolved
@lromor lromor force-pushed the lookahead branch 3 times, most recently from 650c46a to 528c25d Compare February 2, 2022 22:12
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
@lromor lromor force-pushed the lookahead branch 4 times, most recently from 2acddd5 to f962e80 Compare February 8, 2022 20:01
src/planner.cc Outdated Show resolved Hide resolved
@hzeller
Copy link
Owner

hzeller commented Feb 13, 2022

Very nice, the hard deceleration now is completely gone just looking at the make test-html output.

It does seem to accumulate some slight rounding errors along the way, most noticeable in this spiral cut example, in which the new machine path (colored) slightly deviates from the gcode path (solid thin line) at the end of the travel.

Old New
spiral-cut-old spiral-cut-new

Slightly easier to see in this direct comparison

spiral-cut-compare

I think we need some unit test that test this with

  • Very small segments (1 step long). Possibly also intermixed with 0 steps (which should not throw off the planner)
  • switching defining axis between segments. So say given (Axis-X, Axis-Y) we have a segment sequence of (2,1), (1,2), (2,1), (1,2)... and possibly also a test with just one step like in the previous test case (1,0), (0,1), (1,0), (0,1)...

@hzeller
Copy link
Owner

hzeller commented Feb 13, 2022

You might now need to re-base this pull request to incorporate the planner test changes.

@codecov-commenter
Copy link

codecov-commenter commented Feb 14, 2022

Codecov Report

Merging #44 (9485d25) into master (6b2d27a) will increase coverage by 0.86%.
The diff coverage is 98.24%.

@@            Coverage Diff             @@
##           master      #44      +/-   ##
==========================================
+ Coverage   44.34%   45.20%   +0.86%     
==========================================
  Files          38       38              
  Lines        4122     4174      +52     
==========================================
+ Hits         1828     1887      +59     
+ Misses       2294     2287       -7     
Impacted Files Coverage Δ
src/planner.cc 81.87% <98.23%> (+5.80%) ⬆️
src/machine-control-config.cc 77.46% <100.00%> (+0.32%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6b2d27a...9485d25. Read the comment docs.

src/planner.cc Outdated Show resolved Hide resolved
@lromor lromor force-pushed the lookahead branch 2 times, most recently from 688c7db to ad35afb Compare February 23, 2022 23:02
@lromor
Copy link
Contributor Author

lromor commented Feb 24, 2022

Has you suggested, let's add another test checking the behavior for different planning buffer capacities.

Copy link
Owner

@hzeller hzeller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very good!
Here a first round of comments.

src/planner_test.cc Outdated Show resolved Hide resolved
src/planner_test.cc Outdated Show resolved Hide resolved
src/planner_test.cc Outdated Show resolved Hide resolved
src/planner_test.cc Outdated Show resolved Hide resolved
src/planner_test.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
src/planner.cc Outdated Show resolved Hide resolved
@hzeller
Copy link
Owner

hzeller commented Feb 28, 2022

With this change, the test with the square that is compared with single-segment lines vs. multi segment lint

Is now properly accelerating and starting to decelerate in the middle. However, wouldn't we expect the speed profile now be exactly the same ? From the visualization it looks like that the small-segment version accelerates and decelerates quicker, leaving a longer stretch of fast movement ("yellow") in the middle. It looks like the postscript output has the same color/speed legend, so what is different ?

square-moves

@lromor lromor force-pushed the lookahead branch 3 times, most recently from 38560f5 to 8adde18 Compare March 28, 2022 23:01
@lromor lromor marked this pull request as ready for review March 28, 2022 23:01
@lromor
Copy link
Contributor Author

lromor commented Apr 2, 2022

With this change, the test with the square that is compared with single-segment lines vs. multi segment lint

Is now properly accelerating and starting to decelerate in the middle. However, wouldn't we expect the speed profile now be exactly the same ? From the visualization it looks like that the small-segment version accelerates and decelerates quicker, leaving a longer stretch of fast movement ("yellow") in the middle. It looks like the postscript output has the same color/speed legend, so what is different ?

square-moves

I made a small python utility to compare the speed/steps plots of two gcode strings, these are the results for the two ways of doing a straight line:

image

They seem to nicely overlap. I guess gcode2ps doesn't really interpolate the colors based on the speed. It must be doing some linear interpolation if a single segment is provided (instead of using the sqrt function). This is not true for the simulation backend.

@hzeller hzeller merged commit 74b4561 into hzeller:master Apr 7, 2022
@bigguiness
Copy link
Contributor

bigguiness commented Oct 11, 2022 via email

@hzeller
Copy link
Owner

hzeller commented Oct 11, 2022

Is this ready for testing on actual hardware or are you still working on the test code?
I can give it a spin on my hardware when it’s ready and give you feedback on how it physically looks.

Yes, the improved acceleration/deceleration code should be ready now.

Lines with many small segments (such as circles or arcs) that previously would result in an abrupt deceleration at the last segment, are now starting to decelerate early enough to meet deceleration constraints. I've run it on an actual machine and its moves and deceleration are smooth.

Code is slightly slower than before because of more calculations, but that only really shows in direct performance comparisons without a physical machine attached and shouldn't impact real-world speed (though we have it on the TODO list to see what can be improved).

The install document also has been updated to describe how to install BeagleG on a current Beaglebone Debian image.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants