intelligent-agent / redeem

Firmware for Replicape
http://wiki.thing-printer.com/index.php?title=Redeem
GNU General Public License v3.0
36 stars 44 forks source link

Refactor Replicape hardware details into a board detection module #184

Open ThatWileyGuy opened 5 years ago

ThatWileyGuy commented 5 years ago

As a first step towards adding Revolve support, refactor Replicape detection and initialization into its own module.

zaped212 commented 5 years ago

I think ultimately this is a good improvement. It always felt weird that there were board specific configuration knobs inside of the user accessible configuration files.

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.

Wackerbarth commented 5 years ago

Although, I agree with the factoring, I don't agree that it is necessarily the right approach. I definitely think that we should look at this application not as a printer controller, but as a toolbox for building printer controllers. Thus we should make it easy for a user to describe a new board without having to dig into the python code to do so.

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).

ThatWileyGuy commented 5 years ago

Although, I agree with the factoring, I don't agree that it is necessarily the right approach. I definitely think that we should look at this application not as a printer controller, but as a toolbox for building printer controllers. Thus we should make it easy for a user to describe a new board without having to dig into the python code to do so.

I don't think that's desirable or even feasible. Redeem is very tailored to the architecture of TI's AM335x series of SOCs. I'm only aware of two 3d printer control boards made using that SOC, and those are Replicape and Revolve. I doubt there will be a third because the AM335x will likely be obsolete at that point. Furthermore, this implies that a user who can design and build a PCB around an AM335x SOC, build a device tree in the C-like language Linux uses to describe them, tweak U-Boot in C, and debug Linux enough to get the kernel up is also unable to write a board description in Python. In my experience that's just silly - the Python to support the board is written before the board is even complete because the board must be validated.

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?

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?"

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.

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).

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.

Wackerbarth commented 5 years ago

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." -- I can agree only partially. There is no hard requirement that this correspondence be coded in python. The only requirement is that be encoded in the system. Whether that encoding is in procedural code, a table, or some external description file is just an implementation detail.

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.

ThatWileyGuy commented 5 years ago

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.

I don't have a problem with generalizing, but generalizing without a specific target in mind just leads to overengineering.

"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.

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?

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." -- I can agree only partially. There is no hard requirement that this correspondence be coded in python. The only requirement is that be encoded in the system. Whether that encoding is in procedural code, a table, or some external description file is just an implementation detail.

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.

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.

goeland86 commented 5 years ago

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.

ThatWileyGuy commented 5 years ago

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.

ThatWileyGuy commented 5 years ago

Updated to remove all references to replicape_key.

ThatWileyGuy commented 5 years ago

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?

goeland86 commented 5 years ago

@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.