-
Notifications
You must be signed in to change notification settings - Fork 44
Refactor Replicape hardware details into a board detection module #184
base: 2_2_x
Are you sure you want to change the base?
Refactor Replicape hardware details into a board detection module #184
Conversation
redeem/boards/replicape.py
Outdated
return replicape_key | ||
|
||
|
||
def build_replicape_printer(printer, revision): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if this would benifit from creating a new "Replicape_HW" base class, and then inherit from it for the B3, B2, B1, A4, A3, ect. Then we could put general logic in the base class and then only override what is needed in the child classes.
I would probably do multiple levels:
A4 inherits from A inherits from Redeem_HW
That would then allow us to easily add new hw boards, and even create the virtual hw board without much issue at all.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: this suggestion is similar to what was done with the stepper class, though there is some inheritance "problems" in the stepper code where the inheritance is not being fully utilized in that code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider this, but I ultimately decided that the differences were small enough and the overall code was compact enough that it was better to keep it all together to make the differences between revisions more obvious. I suspect this is mostly a matter of taste.
This also won't be an extension point because there are unlikely to be more Replicape revisions.
I think ultimately this is a good improvement. For my A4A board, I had to modify the configuration params to change the Y2 endstop pin in order for it to function correctly due a pin change on later boards. I am glad to see that this will abstract that out and away from the user and has potential to make integrating new boards a lock quicker and easier. |
Although, I agree with the factoring, I don't agree that it is necessarily the right approach. First, it is essential that we abandon the idea that all of the configuration files be stored in the user's directory and, instead, tie the base ones to the install directory. Then those files can be viewed as just an implementation partially written in a language (yaml?) other than python. Then, the question really reduces to one of "which language is more user friendly to specify the kind of things that a user might want to modify?" And along those lines, there SHOULD be a "wiring" layer between the specific hardware interface and its functional use. It would be in this layer that you would designate that the endstop labeled "Y2" is actually a "Z-min probe" (or whatever you are currently using it for). |
I don't think that's desirable or even feasible. Having started looking at what we'd need for Revolve, you have to edit the Python to add support for new peripherals on the SPI and I2C busses and deal with the different ways new stepper drivers handle configuration. Doing it with pure configuration isn't really possible. Are you aware of any other boards we should be supporting?
The python should specify that a given ADC corresponds to a pin on the board labeled "thermistor E." The python should specify that a given MOSFET is connected to heater contacts labeled "heater E." The user should not be able to easily modify this without knowing very thoroughly what they're doing because we cannot offer them help and support if they do so.
In general, we already have this. The one major shortcoming I'm aware of is that while thermistor E and heater E being linked together in a PID controller is a sane default, it should not be mandatory like it is today. |
First, WRT to the SOC, I agree that the Redeem code is very much tied to the AM335x. However, if you look at it from the perspective of functional modules, only a limited portion of the system is actually processor specific. Standard programming practices advocate that those detail aspects be isolated into modules and objects. Thus, substituting a different processor SHOULD still allow a significant portion of the code to be reused. Unfortunately, the present code base has not grown with those design ideas in mind. And there seems to be opposition to changes which only attempt to correct those implementational shortcomings. "Are you aware of any other boards we should be supporting?" -- You seem to have forgotten the "Reach" board and seem to be attempting to drop support for it. You say -- The python should specify that a given ADC corresponds to a pin on the board labeled "thermistor E." The python should specify that a given MOSFET is connected to heater contacts labeled "heater E." -- But the layer of indirection that is currently missing is one which specifies that this particular MOSFET is utilized as the primary extruder. Or that the "X" stepper isn't the one used for vertical motion. When a user can change the attachment point of physical wires to the board, the configuration should include be parameterized to reflect that changeable connection at a single point. |
I don't have a problem with generalizing, but generalizing without a specific target in mind just leads to overengineering.
Reach was a prototype expansion board that never made it to production. If/when the current efforts to revive Reach come to fruition, almost none of that code will be reusable because the new Reach uses different peripherals and stepper drivers than Elias's old prototypes. I judged that it was better to eliminate the code than to let it get more stale than it already is. I'll ask again - do you have a specific other hardware target in mind for Redeem or are you asking that it be generically "generalized" in the hope that something comes eventually?
That doesn't bother me, and I don't think it conflicts with what I've written here. It's also not unreasonable to say that the stepper labeled "X" should be used for the "X" axis by default unless the user takes explicit action to change it. |
I managed to print with this branch with no issues (other than my printer's lack of sufficient part cooling introducing heat curls around edges). For me this refactor is functional. |
It doesn't affect printing, but I do need to make one more update to this PR to fix M115. It's still trying to pull the replicape_key from the config, but it isn't there anymore. |
f8e4ccc
to
6975a32
Compare
Updated to remove all references to replicape_key. |
This PR has been sitting for some time, but it's the first in a series that lead up to Revolve support. What, if anything, can I do to get a consensus? |
@ThatWileyGuy I think we need to have another discussion together with @Wackerbarth and maybe @eliasbakken regarding organization of the hardware abstraction layer we're building. I'd prefer to do that sooner rather than later, but I really want to get @eliasbakken on this, as he's the "primary customer" we should really be making it easier for to hack in new hardware. |
As a first step towards adding Revolve support, refactor Replicape detection and initialization into its own module.