jschoch
commented
1 year ago Thanks for taking the time to explain this. I think some pseudo code would help us each understand our ideas. Stepping back a bit I have to admit i'm very much not a fan of OO and would prefer to do this in something functional like elixir/erlang, so we may be somewhat at odds here. Again, my C++ is very weak but I understand C++ can be more functional if you want.
Regarding having abstractions to swap in/out things that is fine but in my experience with buttons/displays etc it is a huge PITA and running this with the webUI makes dev go very fast and this will be my primary focus. The majority of state flows between the "controller" and the "interface" via arduinoJSON's documents. It is fine if those are structs or object (i'd prefer structs). I don't see enough concurrency for needing getters/setters but maybe you see a case where things need it. The document's need some cleaning up, I don't think i'm using all the values. translating the arduinoJson docs to another format seems like a low overhead thing and should be fairly easy to do.
The FSM currently only does a tiny bit of work, the majority of work is interrupt driven. The encoder ticks, interrupts, and the "gear" calculates the desired position and error and decides if a step is needed. If a step is needed the RTM gets it's instructions and takes care of turning the pulse on/off. there is a tiny bit of logic to ensure that steps get delayed after a dir_pin change.
a lathe that has a 127 tooth metric/SAE conversion gear) that is selectable in a gearboax or something easy to use, you can gain more accuracy and less error, just by setting up the Leadscrew object with the same values, just a different coordinate system and a different ratio and it can figure it out. Can define it as two leadscrews, and swap back and forth between them by their ID in a list. Maybe key the list by measurement system?
Right now the way i'd do this is by adding a bool to the json config object and then making the change during the setFactor() call.
This gets called every time the pitch is changed. BTW rapids are not used directly, it sets the pitch to the rapidPitch and then sets it back when the rapid is complete.
so the UI would issue something like
c.useMetricGear = true; // or c.useImperialGear
var d = {cmd: "jog",config: c}
// send over the websocket (this should be binary msgpack but I think it is just plain old JSON string, another thing to fix)
ws.send(JSON.stringify(d));the commands can be seen here: https://github.com/jschoch/ESPels/blob/master/src/src/web.cpp#L457-L500
It is important to note that the statemachine just sets a flag for the interrupt to read and decide if it should move or not. The big case statment just gets everything ready. When the pos_feeding bool is true the machine typically waits for it's "start" (where ever the encoder happened to be when it was turned on) and when the start is reached it will start trying to figure out if it is where it should be or if it needs to step + or - but only when the encoder interrupt is firing. If the spindle is stopped nothing happens.
So all that said, it is just a tweak to setFactor, I don't think it needs a leadscrew object. perhaps an init, reconfig, and a set function are all that are needed. Once this is set process_motion() and do_pos_feeding() do all the work
https://github.com/jschoch/ESPels/blob/710b00d4c2ba6a7d826f24ede26724b2ffb06f9c/src/src/motion.cpp#L266 https://github.com/jschoch/ESPels/blob/710b00d4c2ba6a7d826f24ede26724b2ffb06f9c/src/src/motion.cpp#L171
Since this is abstracted in "gear" maybe we leave it there and just clean it up a bit? Gear has an init that is called before jogs/rapids. Take a look and see if you think gear is the place to move this logic.
-- Provides important improvements and easy to use mutexes for thread safety
i'm open to making mem access safer, perhaps it is my ignorance but I thought using volatile and IRAM_ATTR was enough. The main things that could have issues are the encoder values, the state, and the control bool flags. I'm not aware of multiple functions trying to write to the same vars, only reads of volatile typed values.
This brings up the naming thing. I started by just trying to get "jog" working but that working code got stuck with the name. It isn't really a "jog", a "jog" should move without the spindle moving and there should be several types of moves.
Jog( speed, distance )where the Z moves without the spindle moving. This is very rarely used.SyncMove( pitch,distance, handedness {CW || CCW})where the Z moves in sync with spindle based on the current "gear" configuration and pitch setting. This assums the spindle will stay running in the same direction and the handedness of the spindle makes it a bit tricky. These moves are always relative to the current position.AbsoluteSyncMove( position )this would move to a "DRO" position based on the current position.- maybe needed
AbsoluteJog(speed,position)
Personally I'd want to tackle this fix first because right now everything is named wrong ;)
digiexchris
Appologies for the long essay. This is an idea of how I'd like to reorganize the code base into C++ style concepts. I can submit PRs with very small proofs of concepts, perhaps converting a single small concept at a time, if that helps it make it clearer. I'll make more issues with ideas to change the machine functionality as I come across them, but this is specifically about the shape of the codebase in general.
I would like to spend some time if you're willing to let me converting some of this into c++ objects and separating the concerns and internal state into logical chunks. I think it would make future development a lot faster by calling getters and convenience functions on instances of an object rather than having to have to know about how the internal state of another area of concern works.
split headers and initializsers into a precompiled header, and then the initializer in the .inl. the PCH can then avoid the huge chain of includes at the top of every file.
For example, one main state machine class that handles the runmodes and other common states that multiple modules use. Other modules call functions to get and change state. Alternatively, an event based system as a later step 2.
Web calls State for basically an array (I'd make a DisplayOutput interface or something), rather than getting DRO position directly from memory. Then we can strip out Web and replace it with another implementation without touching State. Also, it allows us to make alternate interfaces that interact with State with the same api, but over a different method (such as, i2c to a backpack board or something).
I'd like to make some common abstractions, which can be generic, which can be extended by specific use cases. -- Such as, the RMT stepper controls the hardware. That's good. Wrap it in a generic Stepper that extends RMTStepper that takes pin configuration, microstepping, steps per rev etc. at instantiation time -- Leadscrew can either extend Stepper, or have a stepper object in it. I'd prefer Leadscrew to have a MovementDevice which would be an abstract class that is overridden by Stepper. So Leadscrew can instantiate a Stepper, which because of the MovementDevice abstraction, has the same API regardless of wether it's actually a leadscrew, or turning the carriage handle on a rack and pinion, or some other kind of control. Leadscrew can contain things like SAE or Metric, how to convert between the leadscrew system and the stepper coordinate system (sae or metric), and can give instructions to the stepper in distance or steps. -- Machine can have a leadscrew (or more leadscrews, in the case of mill feeding or cross slide feeding or something), and can ask the leadscrew to move a distance in the current coordinate system eg. With a metric leadscrew, and the machine in SAE mode, Machine's Move() can contain myLeadscrewInstance.Move(this.coordinatesystem, "0.001f"); and then the stepper will convert coordinate systems to the stepper system, and then tell the stepper to move a certain number of millimeters. Or steps, alternately. Then Stepper calls rmtStepper functions to do the work.
State -- StateMachine Event ---RunMode ---MoveTo ---Output DRO
Machine -- MovementDevice --- Leadscrew ---- Stepper ----- RMTStepper --- RackAndPinion ---- SomeHardwareImplementation
HumanInterfaceInput interface -- Web -- Buttons -- Nextion interface over i2c
HumanInterfaceOutput Interface -- Web -- i2c Backpack LCD -- Nextion screen presses turned into State Machine commands over i2c
Example workflow: Button press to change to SAE:
And, at runtime or compile time, we can turn on or off all of these features just by constructing the objects differently.
A couple of use cases this helps with:
a lathe that has a 127 tooth metric/SAE conversion gear) that is selectable in a gearboax or something easy to use, you can gain more accuracy and less error, just by setting up the Leadscrew object with the same values, just a different coordinate system and a different ratio and it can figure it out. Can define it as two leadscrews, and swap back and forth between them by their ID in a list. Maybe key the list by measurement system?
It also allows for easily writing a different kind of motor driver, under the MovementDevice contract. Hobby servos, whatever the end user fancies. Or, like we're discussing, replacing the stepper acceleration algorithms. The same api means we can have both in the code and A/B test or even allow the user to select one at config time.
Maybe most importantly, it makes clear distinctions on areas of code and how memory is laid out, which is beneficial to getting more people to contribute. Easier to understand at a glance, easier to unit test in isolation, and easier to submit changes to small chunks of code for small improvements instead of needing a large effort to refactor
-- Provides important improvements and easy to use mutexes for thread safety
Anyway, this is a request for comments with the offer of me doing most of the work if you would like. I don't expect you to drop everything for this :) My web front end skills are weak, but c++ is something I'm familiar with.