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

Add new Kinematic system with skew correction #849

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

elf128
Copy link
Contributor

@elf128 elf128 commented Mar 28, 2023

New system adds ability to tweak skew correction, for cases when physical
geometry of CNC machine is not ideal.
Also, there's implementation of generic abstraction for transformation to-from axis space, it may be used for future implementation of rotated or any other exotic coordinate systems.

Known issues: Machine will do homming in transformed coordinate system as well, which may or may not by desirable. Ideally, config should have an option where user explicitly define homming behavior.

elf128 added 3 commits March 15, 2023 20:18
New system adds ability to tweak skew correction, for cases when physical
geometry of CNC machine is not ideal.

Signed-off-by: Vlad A <[email protected]>
New system adds ability to tweak skew correction, for cases when physical
geometry of CNC machine is not ideal.

Signed-off-by: Vlad A <[email protected]>
Copy link
Collaborator

@MitchBradley MitchBradley left a comment

Choose a reason for hiding this comment

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

The math looks correct but there are a number of stylistic issues that will make the code easier to understand.

Also we need some documentation with examples for how you would describe a machine with specific amounts of skew.

Does it do the right thing if items are left unspecified, i.e. can you write
skewed:
x:
axis_length_mm: 300
y_offset_mm: 2

FluidNC/src/Kinematics/GenericCartesian.cpp Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/GenericCartesian.cpp Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/GenericCartesian.cpp Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/GenericCartesian.cpp Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/GenericCartesian.cpp Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/Skewed.h Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/GenericCartesian.cpp Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/GenericCartesian.h Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/Skewed.cpp Outdated Show resolved Hide resolved
FluidNC/src/Kinematics/Skewed.h Show resolved Hide resolved
@elf128
Copy link
Contributor Author

elf128 commented Mar 31, 2023

Wow, that's a detailed review. Thank you. I'll go over all your points tomorrow.

Meanwhile, I would like to point out couple of things in general.
Yes, axis you don't want to correct can be omitted in config. Everything you didn't specified will be assumed to be zero. In case of empty section, Skewed will act as Cartesian.
As a test, I entered this as a test

kinematics:
  Skewed:
    x:
      dist: 100.0
      z: 0.475
    y:
      dist: 100.0
      z: -1.1

And that's what i've got:

[MSG:INFO: Kinematic system: Skew corrected Cartesian]
[MSG:INFO:       Skew ( 0.000, 0.000, 0.475 ) over 100.000mm]
[MSG:INFO:       Skew ( 0.000, 0.000, -1.100 ) over 100.000mm]
[MSG:INFO:       Skew ( 0.000, 0.000, 0.000 ) over 10.000mm]
[MSG:INFO: Direct transform]
[MSG:INFO:  1.0000  0.0000  0.0047 ]
[MSG:INFO:  0.0000  1.0000 -0.0110 ]
[MSG:INFO:  0.0000  0.0000  1.0000 ]
[MSG:INFO: Reverse transform]
[MSG:INFO:  1.0000  0.0000 -0.0047 ]
[MSG:INFO:  0.0000  1.0000  0.0110 ]
[MSG:INFO:  0.0000  0.0000  1.0000 ]
[MSG:INFO: validation is done] 

Your example will work as you expect, aside from syntax. Yes, I've seen your suggestion about naming and units. You, probably right, but I would like you to take couple of things into consideration.

  1. About units. Actually, this way of definition is unit agnostic. As much, I'm not ever missing the opportunity to piss-off imperial folks and enforce metric here, this definition is distance over distance. Therefor as long as you use same units for all measurements, you should be good.
  2. About axis_length, well, this might be misleading. For example. How would I measure the skew. Let's say, I have a perfectly rectangular piece for aluminium 150mm long. So, I place it on a table and move the tool such as it's touching one corner. Then I move the tool 150mm in one direction and measure, the rise of a tool over known 150mm of travel. That's what goes into the settings. ( Obviously, I'm assuming, that by that time, user already knows which direction cause tool rise, so he/she will do it in the right direction and not plunge the tool into the metal. )
    With naming axis_length this might look like, you have to define actual length of the axis not the distance that been chosen for measurements.

@MitchBradley
Copy link
Collaborator

Re axis_length - point taken.

Re units - you could make the same argument for all FluidNC units that are expressed in mm. The units really should be "GCode units in G21 mode". Conventionally that is taken to be mm, but GCode fundamentally operates in arbitrary units and you just have to be consistent. But that is less understandable to most folks than just calling it mm.

Another approach that would eliminate both problem would be to have "y_factor" which is the dimensionless offset/distance . That would mean that the user has to do a division. Regardless of how it is parameterized, it will be necessary to document the process quite clearly.

@MitchBradley
Copy link
Collaborator

MitchBradley commented Mar 31, 2023

I found this attractive matrix class implementation on stackexchange:

class my_matrix {
  std::vector<std::vector<bool> >m;
public:
  my_matrix(unsigned int x, unsigned int y) {
    m.resize(x, std::vector<bool>(y,false));
  }
  class matrix_row {
    std::vector<bool>& row;
  public:
    matrix_row(std::vector<bool>& r) : row(r) {
    }
    bool& operator[](unsigned int y) {
      return row.at(y);
    }
  };
  matrix_row& operator[](unsigned int x) {
    return matrix_row(m.at(x));
  }
};
// Example usage
my_matrix mm(100,100);
mm[10][10] = true;

Obviously, replace bool with float

@elf128
Copy link
Contributor Author

elf128 commented Apr 1, 2023

OMG, no. This code is a good example how to trash your cache for good.

Imagine how far and inconsistent the pitch will be in this case. There's even no guaranties they will be stored in order. Every time you do matrix operation using data stored like this, you can jump all over the memory.

@elf128
Copy link
Contributor Author

elf128 commented Apr 1, 2023

as for _mm. Ok, I'm sold.

@elf128 elf128 requested a review from MitchBradley April 2, 2023 17:59
@elf128
Copy link
Contributor Author

elf128 commented Apr 10, 2023

Hey,

Did I forgot anything?

public:
Mtx(const size_t row, const size_t col) : _pitch(col), _lines(row) { _buffer = new number[_pitch * _lines]; };

void allocate() {}

Choose a reason for hiding this comment

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

Empty functions, may be get rid of them, or move real allocation and deallocation to those funcs.
Another approach:

  • get rid of those functions and dtor,
  • make buffer in the RAII style
    std::unique_ptr<number[]> _buffer;
    ...
    ctor() {
    _buffer.reset(new number[_pitch * _lines])
    }

@MitchBradley MitchBradley changed the base branch from Devt to main March 21, 2024 17:37
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.

3 participants