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 support for brushless hardware #94

Closed
wants to merge 8 commits into from
Closed

Add support for brushless hardware #94

wants to merge 8 commits into from

Conversation

jedahan
Copy link
Collaborator

@jedahan jedahan commented Oct 4, 2023

Should be ready to merge

  • check real world dimensions are correct
  • fix final pen position after a cancel
  • get the tests to pass

src/planning.ts Outdated Show resolved Hide resolved
src/planning.ts Outdated Show resolved Hide resolved
@jedahan jedahan mentioned this pull request Oct 4, 2023
@jedahan
Copy link
Collaborator Author

jedahan commented Oct 4, 2023

Plan tests fail, gonna have to take another look at it tomorrow unless you have a chance to look at it.

@alexrudd2
Copy link
Owner

It runs pretty well on a RPi 4!

We did notice that the default settings for brushless are pretty slow. It looks like you used the defaults from AxiDraw and not AxidrawFast. This isn't a problem (they can be changed after all), just wanted to make sure you realized.

Got an OOM error that I haven't seen before. No idea if this is new or was triggered by a longer/slower plot (#101).

npm run dev didn't work for me; I used npx . or npm install -g . && saxi. Did not investigate further.

Did not try the UI brushless checkbox; does it work for you?

@alexrudd2
Copy link
Owner

Did not try the UI brushless checkbox; does it work for you?

It works!

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 9, 2023

I'll update the speed values and look at the OOM.

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 9, 2023

Its not obvious to me what parameters need to be updated to speed up the motor - all the past brushless diffs use the same settings. Mind proposing what changes to fix things? (I assume its accel and maxVeloctiy).

I started looking at https://github.com/evil-mad/axidraw/blob/master/inkscape%20driver/axidraw_conf.py and I think it'll take some time to document what params are in what units, and how they map to each other.

Theres also a little confusion of PlanOptions vs Device vs ToolingProfile. Might be reading the wrong fields in some of the code.

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 9, 2023

Some notes regarding the planning test - this is stream of consciousness.

What motions do we expect for a single point at 10, 10? plan([[{x: 10, y: 10}]], AxidrawFast)

Logically I would expect:

up = profile.penUpPos, down = profile.penDownPos

PenMotion(???unknownStartingPosition???,  up)
XYMotion([0,0], [10,10])
PenMotion(up -> down)
PenMotion(down -> up)
XYMotion([10,10], [0, 0])

What do we get if we print the plan?

  [
      { xy: [ { x: 0, y: 0 }, { x: 10, y: 10 } ] }, // move from origin to point
      { z: [ 17750, 15700 ] }, // drop the pen 
      { xy: [ { x: 10, y: 10 }, { x: 10, y: 10 } ] }, // **move from point to point** <-- wat
      { z: [ 15700, 17750 ] }, // lift the pen
      { xy: [ { x: 10, y: 10 }, { x: 0, y: 0 } ] } // move from point to origin
    ]

Can we query the initial position? Should be able to. Might be a TODO in the plan() function in this PR.

Should we always start with a PenMotion to up?

Why do we have an XYMotion with start === end?

@alexrudd2
Copy link
Owner

alexrudd2 commented Oct 10, 2023

Its not obvious to me what parameters need to be updated to speed up the motor - all the past brushless diffs use the same settings. Mind proposing what changes to fix things? (I assume its accel and maxVeloctiy).

I can't figure it out either. My initial guess of Axidraw versus AxidrawFast doesn't make sense since (as you said) the settings are the same. Also the brushed motor remains unaffected.

Perhaps AxidrawBrushless.stepsPerMm isn't correct for the servo? Do your plots have the correct real-world dimensions?

src/planning.ts Outdated
@@ -336,7 +336,7 @@ export class Plan {
// TODO: Remove this hack by storing the pen-up/pen-down heights
// in a single place, and reference them from the PenMotions.
if (j === this.motions.length - 1) {
return new PenMotion(this.minPenPosition, penUpHeight, motion.duration())
return new PenMotion(Axidraw.penPctToPos(0), penUpHeight, motion.duration())
Copy link
Owner

@alexrudd2 alexrudd2 Oct 10, 2023

Choose a reason for hiding this comment

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

We don't want to hard-code this, but I wanted to make it explicit where the logic change had occurred.

Seems relevant: nornagon@6ed9d5d

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

oh! good catch, yeah this should be fixed. Honestly I have not looked into why PenMotions need a starting position in their motion definition - we should be able to know what the current position is since we only ever configure 2 pen positions - up and down - and use that to calculate the target duration delay.

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 10, 2023

Updated the main comment to include checking that real world dimensions are correct.

@alexrudd2
Copy link
Owner

Updated the main comment to include checking that real world dimensions are correct.

Probably relevant: nornagon#120

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 11, 2023

Made some special rectangles, all came out exactly to the mm :)

@alexrudd2
Copy link
Owner

Made some special rectangles, all came out exactly to the mm :)

Congrats and thank you, that's great news!

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 12, 2023

So I tried a more complex svg, and now the stuttering issues are back. also cancel is unresponsive. had to kill the process.

Cannot check if this is a problem upstream or with this patchset since I only have one axidraw.

So we might need to remove the webserial support after all for it to work.

@alexrudd2
Copy link
Owner

So I tried a more complex svg, and now the stuttering issues are back. also cancel is unresponsive. had to kill the process.

Cannot check if this is a problem upstream or with this patchset since I only have one axidraw.

So we might need to remove the webserial support after all for it to work.

Pretty sure it's upstream. And intermittent. :S You can usually see in the console there are lots of connects/disconnects.

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 13, 2023

So I don't see lots of connects/disconnects in the web console, but I was able to reproduce the issue even after restarting saxi with this svg.

9662b7d7-22ba-4e87-b23d-b8422e0c964a

I think this PR is ready, and once its merged I'll start on some more incremental refactoring / formatting PRs.

@alexrudd2
Copy link
Owner

So I don't see lots of connects/disconnects in the web console, but I was able to reproduce the issue even after restarting saxi with this svg.

I can't reproduce with that SVG (on an M1 Macbook Air). That said, I don't see a difference between this branch and main (with a brushed motor). I think the underlying issue is present upstream. See how there are 5 redundant websocket (NOT webserial) messages at once. I've seen this for months and never been able to figure it out
Screenshot 2023-10-14 at 2 38 39 PM

@jedahan
Copy link
Collaborator Author

jedahan commented Oct 14, 2023

Did a little digging by printing out the number of clients when connecting on the server, and a single tab was showing 10 connections. Seems an issue.

@alexrudd2 alexrudd2 closed this Oct 16, 2023
@alexrudd2 alexrudd2 deleted the brushless-v4 branch October 16, 2023 18:40
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.

2 participants