Brushless v3 #87

alexrudd2 · 2023-09-27T02:21:36Z

@jedahan I think the best thing to do at this point is rebase your brushless support on top of main. I don't particularly care about my implementation; it served its purpose showing you a working model.

I've therefore taken jedahan:brushless-v2 and rebased on top of a116669.
This became alexrudd2:brushless-v3.

Please check out this branch and see how you like it.

I consolidated what I could, but still need to resolve:

The two esbuild migrations diverged (Hence rebasing on top of a116669 rather than 18905d8).
Splitting the TS config is still a bit mangled. 37e8b01 was an attempt to fix this but I'm not sure if it's fully what you intended.
We both appeared to try and solve the port.open() problem, so there's a merge conflict with 18905d8

This makes saxi buildable on a raspberry pi zero (webpack OOMs even with 1.5Gb of memory). This also enables live-reload during development.

jedahan

I am requesting changes for myself
Then we can test on real machines and undraft

Thank you for making sense of this large PR

jedahan · 2023-09-27T14:47:17Z

src/ebb.ts

+    console.log('unimplemented')
+    // this.port.close()
+    return


So with this, the firmware-version command writes to stdout but seems to stay open - maybe the error is better UX as at least the command will exit.

Context: nornagon#176

I saw changing from "error" to -> "print firmware version and then error" as an improvement, but clearly the best answer is to fully implement it :)

With reverting WebSerial you fixed here: 2d5c5a1

I think if WebSerial is left in it needs to be await this.port.close()?

jedahan · 2023-09-27T14:53:25Z

src/server.ts

+export async function connectEBB (hardware: Hardware = 'v3', device: string | undefined): Promise<EBB | null> {
+  if (!device) {
+    const ebbs = await listEBBs()
+    if (ebbs.length === 0) return null
+    device = ebbs[0]
  }
+
+  const port = await tryOpen(device)
+  return new EBB(port, hardware)


Maybe device: string | undefined should be named com?: Com here

jedahan · 2023-09-27T14:54:30Z

tsconfig.cli.json

TODO: Confirm that this only builds the cli files.

tsconfig.json

jedahan · 2023-09-27T14:56:35Z

tools/release.js

    demandOption: true
  })
  .parse()

-const newVersion = semver.inc(package.version, args.level)
+const newVersion = semver.inc(packageParsed.version, args.level)


looks like packageParsed might have gotten lost in the merge

src/planning.ts

jedahan · 2023-09-27T15:05:50Z

src/planning.ts

-  public constructor(motions: Motion[]) {
-    this.motions = motions;
+
+  private readonly minPenPosition: number


TODO: Comment on why and how this is used

jedahan · 2023-09-27T15:07:15Z

src/planning.ts

-        // Uuuugh this is really hacky. We should instead store the
-        // pen-up/pen-down heights in a single place and reference them from
-        // the PenMotions. Then we can change them in just one place.
-        if (j === this.motions.length - 3) {
-          return new PenMotion(penDownHeight, Device.Axidraw.penPctToPos(0), motion.duration());
-        } else if (j === this.motions.length - 1) {
-          return new PenMotion(Device.Axidraw.penPctToPos(0), penUpHeight, motion.duration());
+        // 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())
        }


I think this is compensating for the additional pen home command we add to our existing plans. Would love to get a more thorough second look here, is this is a logic change.

nornagon#90

jedahan · 2023-09-27T15:08:16Z

src/planning.ts

      }
-    }));
+      // TODO: CHECK THAT Plan() doesn't overwrite the above motions with this.minPenPosition


TODO: address this TODO

src/planning.ts

jedahan · 2023-09-27T15:20:50Z

I will fix the conflicts and test within two days

alexrudd2 · 2023-09-27T15:47:37Z

I will fix the conflicts and test within two days

Thanks so much. I'm rather new to TypeScipt, after all, and your code is helping me learn as we go. My ultimate goal is to upstream everything into nornagon/saxi and this fork is a staging ground for that.

jedahan · 2023-09-30T23:03:33Z

Alright, finally was able to test on my hardware - there are two known issues currently:

Sending the websocket message {"c":"device","p":{"hardware":"brushless"}} does not seem to change the connected ebb hardware. Consequently, the ui checkbox does not affect anything - you need to make sure to start the server with the correct --hardware flag or the ebb board will send to the wrong pins.
I am seeing some strange behavior with some plots that I think is unrelated to this, but related to me using a rpi zero w that might just be underpowered. Hoping that bun makes a 32-bit build that I can test out to see if its a perf thing.

jedahan · 2023-10-01T17:35:18Z

okay, it was nornagon#131

I am manually reverting the changes in nornagon#63 and seeing if that works (removing webserial support, sadly)

Co-authored-by: Jonathan Dahan <[email protected]>

jedahan · 2023-10-04T20:54:47Z

Going to close in favor of #94, and we can split out any other changes to other PRs

Jonathan Dahan and others added 26 commits September 26, 2023 20:58

Migrate from webpack to esbuild

558cdba

This makes saxi buildable on a raspberry pi zero (webpack OOMs even with 1.5Gb of memory). This also enables live-reload during development.

Wire up model settings in ui

b2239ab

Switch from model -> hardware

1442873

Try and pass in the device path

50840b5

Thread the hardware needle

f584da2

lowercase instances of Axidraw

02e79db

Fix some tsc lints

db3a67c

pass the hardware option in cli plot command

2e5860b

Appease the type checker

7be60a9

Minify js

1825f53

Pass com path back in broadcast info

bb0104d

Simplify path generation for pen up, fix tsc errors

abd9591

Try satisfying the same hack for withHeight

0fee7ca

Fix tsc errors, run format

37c65ed

Split typescript config for editor support

97958df

Move --firmware-version from option to command

12ad5a1

exclude more, es2020

94106b9

Try starting with strict mode

10c8960

nvm

2e5cf08

f typescript

2a0045c

Add back firmware-version

2022f86

Skip library checking

f0eafd5

Make sure to open the port first

f3b889c

Try implementing this.port.close

e1fa8da

try not throwing

847e304

Fix scripts

37e8b01

alexrudd2 marked this pull request as draft September 27, 2023 02:24

alexrudd2 mentioned this pull request Sep 27, 2023

Refactor brushless support #86

Closed

jedahan requested changes Sep 27, 2023

View reviewed changes

alexrudd2 added 2 commits October 2, 2023 21:39

fixup! Fix tsc errors, run format

00383e4

fixup! Simplify path generation for pen up, fix tsc errors

da405c4

alexrudd2 force-pushed the brushless-v3 branch from 07d142d to da405c4 Compare October 3, 2023 02:40

Jonathan Dahan and others added 3 commits October 2, 2023 22:32

Skip typescript build for web

85b8a2f

fixup! Fix scripts

5a7aa1a

Co-authored-by: Jonathan Dahan <[email protected]>

Switch base config to expected name for editor support

d300952

alexrudd2 force-pushed the brushless-v3 branch from a5c7ae5 to ff38c06 Compare October 3, 2023 04:23

alexrudd2 and others added 2 commits October 2, 2023 23:29

fixup! Switch from model -> hardware

6678432

Fix test logic, but now the tests show different values for penPos

155ad2b

alexrudd2 force-pushed the brushless-v3 branch from ff38c06 to 155ad2b Compare October 3, 2023 04:29

jedahan closed this Oct 4, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Brushless v3 #87

Brushless v3 #87

alexrudd2 commented Sep 27, 2023

jedahan left a comment

jedahan Sep 27, 2023

alexrudd2 Sep 27, 2023

alexrudd2 Oct 3, 2023

jedahan Sep 27, 2023

jedahan Sep 27, 2023

jedahan Sep 27, 2023

jedahan Sep 27, 2023

jedahan Sep 27, 2023

alexrudd2 Oct 3, 2023

jedahan Sep 27, 2023

jedahan commented Sep 27, 2023

alexrudd2 commented Sep 27, 2023

jedahan commented Sep 30, 2023

jedahan commented Oct 1, 2023

jedahan commented Oct 4, 2023

Brushless v3 #87

Brushless v3 #87

Conversation

alexrudd2 commented Sep 27, 2023

jedahan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jedahan commented Sep 27, 2023

alexrudd2 commented Sep 27, 2023

jedahan commented Sep 30, 2023

jedahan commented Oct 1, 2023

jedahan commented Oct 4, 2023