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.

Sign up for GitHub

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

Use initial pen up value on cancel instead of an arbitrary value #163

Merged
merged 2 commits into from
Feb 16, 2025
Merged

Use initial pen up value on cancel instead of an arbitrary value #163

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e517b8
Use initial pen up value on cancel instead of an arbitrary value of 50
drskullster Feb 10, 2025
a6b0808
Merge branch 'main' into fix-pen-height-on-cancel
drskullster Feb 16, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 6 additions & 8 deletions src/server.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import type { PortInfo } from "@serialport/bindings-interface";
import { WakeLock } from "wake-lock";
import WebSocket from "ws";
import { SerialPortSerialPort } from "./serialport-serialport";
import { Device, PenMotion, type Motion, Plan } from "./planning";
import { PenMotion, type Motion, Plan } from "./planning";
import { formatDuration } from "./util";
import { autoDetect } from '@serialport/bindings-cpp';
import * as _self from './server'; // use self-import for test mocking
Expand Down Expand Up @@ -158,7 +158,7 @@ export async function startServer (port: number, hardware: Hardware = 'v3', com:
interface Plotter {
prePlot: (initialPenHeight: number) => Promise<void>;
executeMotion: (m: Motion, progress: [number, number]) => Promise<void>;
postCancel: () => Promise<void>;
postCancel: (initialPenHeight: number) => Promise<void>;
postPlot: () => Promise<void>;
}

Expand All @@ -170,10 +170,8 @@ export async function startServer (port: number, hardware: Hardware = 'v3', com:
async executeMotion(motion: Motion, _progress: [number, number]): Promise<void> {
await ebb.executeMotion(motion);
},
async postCancel(): Promise<void> {
const device = Device(ebb.hardware);
Copy link
Owner

Choose a reason for hiding this comment

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

@drskullster did removing this line improve anything? Were there any side effects?

(I don't know its original purpose)

Copy link
Author

@drskullster drskullster Feb 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@alexrudd2 from what I understand the Device object was necessary to get the machine value for a specific position (device.penPctToPos(50)). Since we now pass the position from the first pen motion on line 225 this is not needed anymore.

// TODO: switch to pen up position
await ebb.setPenHeight(device.penPctToPos(50), 1000);
async postCancel(initialPenHeight: number): Promise<void> {
await ebb.setPenHeight(initialPenHeight, 1000);
},
async postPlot(): Promise<void> {
await ebb.waitUntilMotorsIdle();
Expand All @@ -189,7 +187,7 @@ export async function startServer (port: number, hardware: Hardware = 'v3', com:
console.log(`Motion ${progress[0] + 1}/${progress[1]}`);
await new Promise((resolve) => setTimeout(resolve, motion.duration() * 1000));
},
async postCancel(): Promise<void> {
async postCancel(_initialPenHeight: number): Promise<void> {
Copy link
Owner

Choose a reason for hiding this comment

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

The 2 implementations should be kept in sync. (See #164 (comment)).

Copy link
Author

Choose a reason for hiding this comment

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

@alexrudd2 so this is the simPlotter object that as far as I understand doesn't implement any action and just logs them. Interestingly the webserial implementation already checks for the pen up original position (or falls back to 50):

saxi/src/ui.tsx

Line 185 in 03df63e

const penUpPosition = penMotion ? Math.max(penMotion.initialPos, penMotion.finalPos) : device.penPctToPos(50);

Copy link
Owner

Choose a reason for hiding this comment

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

Good point on the simPlotter.

Regarding the WebSerial implementation, that's probably the more robust implementation. The Math.max is screwy, but is actually intended to make things work with reversed output pins (where commanding the pen up actually moves it down),

console.log("Plot cancelled");
},
// eslint-disable-next-line @typescript-eslint/no-empty-function
Expand Down Expand Up @@ -224,7 +222,7 @@ export async function startServer (port: number, hardware: Hardware = 'v3', com:
motionIdx = null;
currentPlan = null;
if (cancelRequested) {
await plotter.postCancel();
await plotter.postCancel(firstPenMotion.initialPos);
broadcast({ c: "cancelled" });
cancelRequested = false;
} else {
Expand Down