Rigidbot support

[thinkyhead]

• Redraft of RigidBot support #1063 / Adding support for Invent-A-Part RigidBot #2387 / Adding support for RigidBot #2389

[Wackerbarth]

@emartinez167 - It you can test this, it should be ready to merge.

[emartinez167]

Test passed. Please merge.
On Thu, 9 Jul 2015 at 18:30 Richard Wackerbarth notifications@github.com
wrote:

@emartinez167 https://github.com/emartinez167 - It you can test this,
it should be ready to merge.

—
Reply to this email directly or view it on GitHub
#2401 (comment)
.

[emartinez167]

Thank you all!
On Fri, 10 Jul 2015 at 02:36 Richard Wackerbarth notifications@github.com
wrote:

Merged #2401 #2401.

—
Reply to this email directly or view it on GitHub
#2401 (comment).

[emartinez167]

Apologies, I just found that some lines were missing (I should have tested on a clean Development branch - The lines are in my own dev branch). Not sure why they were not included in the PR, as I cannot find them anywhere else.

Marlin.h, line 339:

// RigidBot Feature
#ifdef STEPPER_RESET_PIN
  void disableStepperDrivers(void);
  void enableStepperDrivers(void);
#endif

Marlin_main.cpp:
Line 604:

// RigidBot Feature
  #ifdef STEPPER_RESET_FIX
     disableStepperDrivers();
  #endif

Line 659:

// RigidBot Feature
  #ifdef STEPPER_RESET_FIX
     enableStepperDrivers();
  #endif

Line 935:

// RigidBot Feature
#ifdef STEPPER_RESET_PIN
  void disableStepperDrivers() {
    pinMode(STEPPER_RESET_PIN, OUTPUT);    // set to output
    digitalWrite(STEPPER_RESET_PIN, LOW);  // drive it down to hold in reset motor driver chips
    return;
  }

void enableStepperDrivers() {
    pinMode(STEPPER_RESET_PIN, INPUT);     // set to input, which allows it to be pulled high by pullups
    return;
  }
#endif //STEPPER_RESET_PIN

[gregrebholz]

Probably obvious what I was trying to accomplish with that, but in case not - the RigidBot controller uses a capacitor near !RST on the stepper drivers, and because of their charge time during initial power up, the drivers can be left in an undetermined state if they receive power on Vcc while !RST is at some arbitrary voltage. This "occasional dead axis that requires a board reset" was discovered after manufacturing, so a rework was done to run a wire from a micro controller pin to the !RST pin of all the drivers. The firmware then forces the drivers low and releases them after the capacitors are charged.

So, it's a required bit of code for RigidBot owners, but I like Scott's suggestion that all the ifdefs be changed to look for STEPPER_RESET_PIN instead of yet another Configuration.h setting, STEPPER_RESET_FIX.

I also found the same capacitor near !RST design on at least one other RAMPS derivative controller but the name is eluding me at the moment. So this is theoretically a feature that other designs may avail themselves of in the future.

[Wackerbarth]

@gregrebholz writes "I like Scott's suggestion that all the ifdefs be changed to look for STEPPER_RESET_PIN"

HAS_STEPPER_RESET_PIN would be a name that better describes the function ("FIX" is rather vague)

@thinkyhead -- I think that Greg may be suggesting that the existence of a definition of a STEPPER_RESET_PIN is sufficient to activate the code. If that were your intention, I don't know why you would suggest it. Not only is it a bad idea, but it goes directly against the logic behind the "ENABLED/DISABLED" macros that you have proposed.

Remember that, although a pin may be allocated for a purpose, that does not always imply that it is being used for that purpose. Thus the enabling of a feature should ALWAYS have an enabling flag for the optional feature and that flag should be true/false rather than something like the value of a pin.

[gregrebholz]

I think it was suggested, and I was supporting it, to declutter the configuration a bit, as 99% of the user base will not be using it. This pin declaration was introduced by this supporting code, so there are no circumstances where a board has declared a "STEPPER_RESET_PIN" macro but intends to use it for something else. If a future board wants to declare this same macro but desires to use it for some other purpose then there would be a feature worthy of a toggle in the configuration.

If this were a "if the board had a buzzer pin, buzz it now" function, you want to have a "use buzzer" configuration toggle to opt out, even though your board supports it. Here opting out (which would have to be the default for the 99%) means you will randomly experience axes that don't move. Pivoting on the existence of the pin definition seems appropriate for this "mandatory behavior" if your board requires it.

[Wackerbarth]

I’m all for decluttering the Configuration file, not just a little, but a lot. However, this is a somewhat separate issue of overloading the specification of a pin access. I’m objecting to having a non-boolean “selector” for the feature.

There is no reason that ANY of the unused “options” have to be in any of the active configurations.

Additionally, rather than defining just the pin access number, there is no reason why, also, you cannot define the “HAS_STEPPER_RESET_PIN” selector within the board’s definition file.

That way, within the main marlin code, you are not tying the stepper reset function to a particular implementation of StepperDrivers.

In your description below, you have things backward. It’s not “for some other purpose”, but “some other implementation”. So, "Pivoting on the existence of the pin definition” is NOT appropriate. Rather, “Defining the feature within the board specific configuration file rather than in the user configuration file” is the appropriate mandate.

On Jul 10, 2015, at 12:22 PM, gregrebholz notifications@github.com wrote:
I think it was suggested, and I was supporting it, to declutter the configuration a bit, as 99% of the user base will not be using it. This pin declaration was introduced by this supporting code, so there are no circumstances where a board has declared a "STEPPER_RESET_PIN" macro but intends to use it for something else. If a future board wants to declare this same macro but desires to use it for some other purpose then there would be a feature worthy of a toggle in the configuration.

If this were a "if the board had a buzzer pin, buzz it now" function, you want to have a "use buzzer" configuration toggle to opt out, even though your board supports it. Here opting out (which would have to be the default for the 99%) means you will randomly experience axes that don't move. Pivoting on the existence of the pin definition seems appropriate for this "mandatory behavior" if your board requires it.

[thinkyhead]

"Pivoting on the existence of the pin definition” is NOT appropriate

The presence of a "stepper reset" pin is very unusual. So far as I have seen, only RigidBoard has it. If it is defined for a board, it makes sense to use it in that case. It's consistent with the way that fan code is compiled-in whenever there is a FAN_PIN defined. But do we imagine there will be boards that have this pin but which don't want to use it?

[thinkyhead]

There is no reason that ANY of the unused “options” have to be in any of the active configurations.

Except, the root configuration files at least, which are meant to be reference configurations with every (non-delta / non-scara) option, documented with comments. Other configurations (i.e., example_configurations) currently duplicate every option, even if disabled, leaving it open for users to adopt any options they need as they customize their machines, change electronics, etc. Having them all in one place, with "available" options simply commented-out, can be considered a convenience in some respects. Diff'ing any two Configuration[_adv].h files produces a usable comparison, for example.

[thinkyhead]

#2417 follows the convention that when this pin exists, it is reset at startup.

[Wackerbarth]

I again find this to be a poor convention and feel that it should be eliminated because it assumes a particular implementation for the feature. In its place, we should enable the feature in the board file and define the "pin" that is used for the current implementation. The same holds for FAN_PIN. The enabler for the code should be boolean and the parameter that associates it with a particular mcu register could be in any appropriate namespace.

[Wackerbarth]

As for having all of the options in EVERY configuration file, the practice obscures the actual configuration because of the unselected options, many of which cannot apply, and the "documentation" of the possibilities causes the file to become "unreadable".
From my observation, in use, the file gets stripped to remove the irreverent comments and leave only the parameters which apply to the actual configuration.
We do need to have a reference document which describes each of the various options and the parameters associated with them. But, attempting to keep all of the example files in sync, including the unused options, has proven to be fraught with error. And, as to doing comparisons, the difference between two configurations is the same whether they contain lines commented out, or not.
(Actually, I can argue that the difference would be "cleaner" without the lines that are commented out)

[gregrebholz]

I understand what you're saying with respect to conventions and overloading the meaning of a pin definition, but consider that your alternative would 1) define a configuration option in a board's pin definition file for the first time in this project, and 2) create a configuration option which will only be true or undefined, never false. The alternative for 1 is mysterious problems for RB users, and for 2 having every board that doesn't use a stepper reset pin (all of them except RB) state as much in their pin definitions.

This feature is moving from zero implementations of a "stepper reset pin" to one, and I think the introduction of a second implementation would be a good time to introduce an actual configuration option. In fact the author of such a pull request would have to include as much to resolve their reuse of an existing macro name for a new implementation.

Regarding including every option in a config file I've seen it done both ways, and agree there's a tipping point in scale when it becomes unwieldy. Marlin is certainly close to that point.

[Wackerbarth]

@gregrebholz -- With respect to your last comment.

This is not a MACRO. It is a definition of a property.
If you insist in "automating" the setting of the property based on the existence of a definition for that pin, then look to add something to "Conditionals.h", That is exactly what that file does for other properties.

However, I will agree that it is a property of the board, and, therefore, the board-specific file is the proper place to set it.

As far as "true or undefined" is concerned, this is in no way different from the way we handle any of the "user" options. I think that your argument in the first paragraph is incorrect. By defining the property, either directly in the board-specific file, or in the Conditionals.h file, I don't see any difference for the present boards.

It is an issue of opaqueness. If it were not for the fact that it is difficult to have the compiler/linker recognize and optimize away routines that "do nothing", and we are still attempting to function in an environment where a few bytes of ROM are important, we should be "calling" this reset function for every implementation. Then, hidden down in the routine, boards which do not have the functionality would simply return.

Further, I disagree with the attitude that we should continue to add instances of a bad coding style simply because we have other instances that we are not removing (at this time). Neither style is mutually exclusive. We can successfully compile the firmware when it has a mix of styles. In fact, incrementally improving the style is a viable approach to removing the antiquated style.

BTW, your code (above) references two different properties related to the same feature. Perhaps better naming conventions would make it easier to spot such errors.

[gregrebholz]

I apologize for my sloppy use of the word "macro" for the C preprocessing directive #define. Mea culpa.

My code used two different properties because I wrote it the way you are suggesting is the only correct way. RB users would define STEPPER_RESET_FIX in Configuration.h to enable the code, and a STEPPER_RESET_PIN in pins.h. I prefer the overloading of _PIN suggestion for the reason already stated.

Your description of this style as "bad" is a matter of opinion, and reasonable minds can both agree and disagree with you. There is nothing about the proposed overloading suggestion that confuses me in the least. The presence of such a pin on a board necessitates its use. There is no "I have a stepper reset pin but I don't want to use it" scenario. Your suggestion of a future "I have a stepper reset pin but I don't want to use it in the current way it is used, and instead use it in this alternative implementation" scenario could happen, but it will be accompanied by that second implementation (perhaps pulling a pin high instead of low) and the necessary changes to allow choice between the two will be included in that.

I'm not familiar with a Conditionals.h file, where is it? I'm assuming you are proposing a compromise like: "#ifdef STEPPER_RESET_PIN; #define HAS_STEPPER_RESET; #endif" and then the #ifdef in the code pivots on HAS_(feature) as you desire? This seems like needless complexity to me, and thus more opaque, but it would both address your desire for a feature Boolean and my desire for RB users to always get the reset functionality they need.

[Wackerbarth]

My point about "not a macro" is that the pins.h file is already a file #defining a bunch of properties of the board.

This is not so much a "I have a stepper reset pin", per se, but rather "my configuration requires the stepper reset function". The main code loop should not care HOW that is implemented. It need only know that it should include the function calls at those critical places. Were code size/speed not an issue, I would ALWAYS call the routine (and allow it to be a nop for most configurations).

As for the naming, I strongly prefer the use of names that directly express the intent. STEPPER_RESET_PIN is, to me, a physical entity. The code (in the main loop) calls some initialization function. Hence the "HAS_(the_function)" property.

As for assuring that RB users get this desired call, I think that it is most appropriate to include the specifier in the board file. I would provide #define HAS_STEPPER RESET
#define STEPPER_RESET_PIN XX That way, we have the same result. Either the terms are defined, or they are not.

[thinkyhead]

my configuration requires the stepper reset function

This is board-level, so "my board has a STEPPER_RESET_PIN."

#define HAS_STEPPER RESET

Already defined in Conditionals.h (in the PR) as:

#define HAS_STEPPER_RESET (PIN_EXISTS(STEPPER_RESET))