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

Brushless checkbox only changes plan options, not EBB connection #126

Open
alexrudd2 opened this issue Feb 6, 2024 · 6 comments · May be fixed by #133
Open

Brushless checkbox only changes plan options, not EBB connection #126

alexrudd2 opened this issue Feb 6, 2024 · 6 comments · May be fixed by #133

Comments

@alexrudd2
Copy link
Owner

nornagon#195 had a case where starting the app via npm start, and then later clicking the 'brushless' checkbox in the UI didn't apple the changes to switch to brushless. Writing a test to catch this case would be useful, and fixing whatever is broken.

originally posted by @jedahan in nornagon#202 (comment)

@alexrudd2
Copy link
Owner Author

alexrudd2 commented Feb 6, 2024

I can reproduce. The PlanOptions and so on get changed by toggling the checkbox. However, there's currently no way of changing the underlying EBB driver. It gets initialized once and then can't be changed.

There is mismatch in data flow.
The UI checkbox is connected to planOptions.hardware in ui.tsx and planning.ts
The CLI argument is connected to ebb.hardware in ebb.ts and server.ts.
The two hardware properties are not connected to each other.

The EBB device info used to be sent all the way up to the UI (in nornagon@0e9cd16). However, that broke with the refactor in 3e89b85, since the new getDeviceInfo() change the signature.

@alexrudd2
Copy link
Owner Author

@jedahan does my explanation make sense? any ideas how to clean it up?

@jedahan
Copy link
Collaborator

jedahan commented Feb 18, 2024

I had a draft comment from a while ago, but I will need to spend some time re-familiarizing myself with the code to see if this is off-tangent or relevant. Expect a followup in ~3 days


Some ideas for refactors. There are a few things going on here - one is properly bringing hardware outside planOptions in the UI (this might have just been a hack for me to re-use existing codepaths, instead of understanding the UI framework better), and a way to move initialization of things like pin settings to runtime instead of "build" time.

The second thing is that I think the word hardware is overloaded. We could jam on a document to explore options, but here is just one way to slice things, to get that discussion started, as an example:

  • board - should be consistent across plotters that conform to the ebb command set, and include the ability to communicate between the host and board

  • toolhead - brushless vs non-brushless, any other pinouts we care about (do speed setting live here as well?)

  • plotter - board + toolhead (this could be presets, or its own model, and/or be called 'hardware') **(do speed settings live here?) **

  • settings - settings for a particular plot

  • plot - source image + settings

  • plan - moves sent to plotter by applying settings to a vector image


@alexrudd2 alexrudd2 linked a pull request Apr 20, 2024 that will close this issue
@alexrudd2
Copy link
Owner Author

I took another look and created a (very rough) PR that does manage to change the underlying ebb.hardware (i.e. it changes the pin used). Not sure how best to combine with the planner and React checkbox,

@alexrudd2
Copy link
Owner Author

The second thing is that I think the word hardware is overloaded. We could jam on a document to explore options, but here is just one way to slice things, to get that discussion started, as an example:

Good point. What about keeping things simple and just using motor?

@alexrudd2
Copy link
Owner Author

I took another look and created a (very rough) PR that does manage to change the underlying ebb.hardware (i.e. it changes the pin used). Not sure how best to combine with the planner and React checkbox,

Yeah, I realize there is currently no mechanism that updates the React checkbox, since it's storing its state rather than getting it from the EBB.

state.deviceInfo.hardware will have information on the EBB.... eventually. The checkbox is drawn before the websocket message dev is sent and ondevinfo() fires.

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 a pull request may close this issue.

2 participants