MaslowCNC / Firmware

Maslow Firmware
GNU General Public License v3.0
260 stars 136 forks source link

AUX pins should be defined at runtime #417

Closed blurfl closed 6 years ago

blurfl commented 6 years ago

Various versions of PowerControl board have had different AUX pin connections. Presently they are configured using #define in Config.h To date we have used the AUX pins which did not change between versions of hardware. In order to make use of all the AUX pins including the ones that change between boards, it seems like they should be defined at runtime based on the board version much as the INx, ENx and ENCODERxy pins are. Is there a clean way to do this without needing to re-write Spindle and Probe as classes in the way that Axis is done?

BarbourSmith commented 6 years ago

Hmmm that's a good question.

I agree that the AUX pins should be defined at run time because we will want to move them in the future.

From a quick look at the code I can't see a way to do it other than how the axies are done, but it looks like both of those classes were designed with the pins as variables so it shouldn't be too hard to switch them over

blurfl commented 6 years ago

I see that I was the one who originally #defined the AUX pins in PR #223 almost a year ago (blush, shame) and it didn't account for AUX5 and AUX6. I agree that the class approach is doable, I just hoped for a simpler one.

aprospero commented 6 years ago

To be honest, the Probe files would have been on my list for deleting to simplify the program flow (see #415). The single only function just wraps another function call...

blurfl commented 6 years ago

the Probe files

They represent an organization that made sense to the one who made it. Separating functions in that way made it easier to find things, a boon in my opinion. As one of our fundamental goals is to adopt/merge as much of the grbl firmware functionality, our code was restructured to mirror that project's organization. That has helped to understand the differences and similarities. I would be disappointed to see our code become harder to carry toward grbl compatibility.

aprospero commented 6 years ago

Ah, thanks to clarify that! This long term goal was not obvious to me. I'll have a deep look into grbl before I do anything stupid. :)

blurfl commented 6 years ago

Not stupid, but your effort could take us further toward that goal as well. Refactoring our code to make grbl sections 'drop in' could be a big advantage.

aprospero commented 6 years ago

Agreed. And this could be the solution for finding common Coding guidelines as well. We could take grbl as blue print. their code seems to be well formatted - at least on first glance.

aprospero commented 6 years ago

Hmmm, maybe I should ask this elsewhere, in the forum or in the wiki. How far are you planning to adopt the grbl code structure? In my understanding the math of maslowCNC is completely different to grbl, starting with servo vs. stepper and not ending with the peculiarities of the maslow sled. A lot of layer concepts like the planner or the protocol layer could be rewritten though, but I doubt that we can just copy paste code in a big way. If this is correct and we have to rewrite most of the code anyway, why should we copy the grbl code structure? For sure, they did their homework - but that doesn't mean that their concepts are in every detail as good for us as for them. I can imagine that the idea was appealing when it came to adapting gcode parsing, but in my understanding that task is mostly done and maslow has already most important/necessary gcodes adapted. So what is the benefit of using grbl as blueprint?

Just want to understand the intention...

blurfl commented 6 years ago

ask this elsewhere, in the forum or in the wiki. How far are you planning to adopt the grbl code structure?

It certainly belongs in a place by itself. Not sure if the forum is appropriate - an issue here in Firmware along with a post directing those interested to it perhaps. @krkeegan has a deep understanding of the code base and has been leading the way toward grbl compatibility. His insight will be very valuable.

blurfl commented 6 years ago

the planner or the protocol layer could be rewritten though

If someone was looking for a place to make a huge contribution, those would be great places to look. I've heard the opinion that the Maslow motor control and the requisite math portions could be compartmentalized and arranged to receive instructions from the planner/protocol modules in the same way that the stepper portion of grbl does. That would be the 'drop in' approach.

aprospero commented 6 years ago

Ah! I just had a glance on the grbl trigonometry and the interface of the planner yet. I assumed that the principle of the closed loop would not comply with stepper motor controls. Thanks a lot for your comments, this helps me a lot in understanding what's the matter here! I like to be of help and just landed at the first place I've seen it seemed in need for.

blurfl commented 6 years ago

I've found a way to define the AUX values at runtime without re-writing Probe and Spindle. Setting SpindlePowerControlPin and ProbePin in the same way that the INx, ENx and ENCODERxy pins in a function in System.cpp, and declaring them as extern works. I'll craft a PR after the next bump.

davidelang commented 6 years ago

On Mon, 26 Mar 2018, aprospero wrote:

Hmmm, maybe I should ask this elsewhere, in the forum or in the wiki. How far are you planning to adopt the grbl code structure? In my understanding the math of maslowCNC is completely different to grbl, starting with servo vs. stepper and not ending with the peculiarities of the maslow sled. A lot of layer concepts like the planner or the protocol layer could be rewritten though, but I doubt that we can just copy paste code in a big way. If this is correct and we have to rewrite most of the code anyway, why should we copy the grbl code structure? For sure, they did their homework - but that doesn't mean that their concepts are in every detail as good for us as for them. I can imagine that the idea was appealing when it came to adapting gcode parsing, but in my understanding that task is mostly done and maslow has already most important/necessary gcodes adapted. So what is the benefit of using grbl as blueprint?

Just want to understand the intention...

well, in my ideal world, we would make it so that grbl supported servos as well as steppers (ideally in a per-axis config), and then the maslow math could become a different mode of operation (similar to coreXY), and then grbl would completely replace the maslow firmware

:-)

There are a lot more people working on grbl than are working on maslow, and if we could just focus on the parts that are maslow specific, it would benefit us a lot.

there is a desire in the grbl community to be able to support servo operation (DC motor+encoder as maslow uses, hobby-class servos with write-only PWM control, and BLDC motor+encoder uses), getting this sort of support added should be relatively easy (although it may not be able to go in the 1.x arduino version, it may be limited to the 2.x tree that supports more advanced processors)

the maslow math will be more of an uphill argument, but if we make it so that it's a clean compile-time option, we have a chance.

David Lang

davidelang commented 6 years ago

On Mon, 26 Mar 2018, aprospero wrote:

Ah! I just had a glance on the grbl trigonometry and the interface of the planner yet. I assumed that the principle of the closed loop would not comply with stepper motor controls.

closed loop vs stepper are very different and would require completely re-writing the main control loop of grbl, but since they would both be receiving instructions from the planner, it should not be impossible to have both types of control loops available.

David Lang

aprospero commented 6 years ago

Thanks for your comments David,

it helps a great deal understanding the desired roadmap. After some evaluation I admit I'm not really convinced that making maslow firmware structure similar to grbl is really the next step to go. It seems there are already people working at closed loop PID control support for grbl. As you stated the demand for it is already high and just the fact that RAM/PROM resources on Arduino Uno are already used up seems to keeps them from including it right away. Am I right assuming this is what you are talking about when you say 'the 2.x tree that supports more advanced processors'? (I haven't found that yet, I would be happy if you could give me a hint in the right direction.) IMHO it's better waiting without making the maslow firmware more complicated than necessary, as we don't know the interface we'll have to adapt to, yet. Meanwhile we could work on cleanliness, optimization and new features of the modules that definately will be part of a future merge. Please correct me if I miss anything!

Don't get me wrong, I'm the last one who likes to talk down things without any effort by myself. But I feel I owe you an explanation why I likely won't contribute much in that direction.

All the best,

       Tim
blurfl commented 6 years ago

Addressed in PR #419