RigidBot support #1063
RigidBot support #1063
Conversation
RigidBoard controller, the RigidPanel LCD with USB drive support, and automated build scripts for various configurations of the RigidBot
language options, fixed MSG_CONTRAST in a number of language (removing duplicate definitions and adding a missing one for Russian) Needs translation for other languages.
| @@ -303,83 +312,207 @@ your extruder heater takes 2 minutes to hit the target on heating. | |||
| // Uncomment the following line to enable CoreXY kinematics | |||
| // #define COREXY | |||
|
|
|||
| // coarse Endstop Settings | |||
There was a problem hiding this comment.
Moved to "custom mechanics" block at newfile line 397
|
Personally I don't like introducing a big if/else block in Configuration.h, though I really do like the idea of predefined values for known machines. |
|
Thanks for looking at my request oysteinkrog. I agree with you, and just pushed commit #a06578e that moves this structure out to a mechanics/ subdirectory where we can define other machines. In cases like the rigidbot where there are two "sizes" that affect build volume, the mechanics/*.h files can include if/else structures that keep them generic and Configuration.h can remain a one-stop-shop for defining your configuration, as in "#define RIGIDBOT_SIZE 1" in my implementation. Thank you again for the feedback. |
|
+1 for pre-made configuration files for various machines, with a switch in configuration.h. Configuration.h already has an ifdef at the top, so all you would need to do is add your customized configuration.h with its own name (I would do a folder of them to reduce clutter), then include your printer's configuration file from the top of configuration.h, before the ifdef. The other config would be interpreted first and the "#define CONFIGURATION_H" line in it would prevent the rest of the default configuration.h from being read. |
|
Thanks whosawhatsis. Have you seen the most recent commit that created a new mechanics/ directory to hold what I had previously put in an ifdef block? This brings the change to Configuration.h down to two lines and a comment block explaining their use. I think the pre-existing Marlin/example_configurations/ directory is probably the right place for entire Configuration.h files which can be copied over the Configuration.h in the source directory before compiling, but I was going for something more granular here. The idea that you might buy a RigidBot kit, thus determining your build volume, acceleration, jerk, endstops, etc. but you might decide to include runaway heater protection or not, or add a auto bed leveling probe, or put an e3d v6 hotend on it and want different thermistor profiles... but since the mechanics of your kit remain unchanged, you are able to select all of them with one line in the Configuration.h If this isn't a welcome addition I can change to having an example Configuration.h for the RigidBot in example_configurations/ ... but it means that when new features like the recent "Z probe sled" are added to Configuration.h, all the various example_configurations/*/Configuration.h files will need to be updated to avoid undefined errors. Not the end of the world, but if 3d printer kits of fixed dimensions become a popular option it may be nice for folks to select their kit in the same way they select their "MOTHERBOARD" now, and focus only on turning features on/off in Configuration.h |
|
Just looked at it, and I don't like it. You shouldn't have to run a shell script to do this, just work within the existing preprocessor system. Including a different configuration file at the top of configuration.h will override everything in that file, and there's nothing stopping you from modifying that configuration file. Using a shell script to do this is a much bigger burden, especially on Windows, which IFAIK, can't even run a bash script. |
|
Sorry, you're looking at the wrong thing. That .sh file is an automated build script that makes several firmwares by substituting some options in and out between compiles. It's really only an example that other communities may want to follow if they intend to build lots of pre-compiled firmwares for their group. Everything we're talking about here is Configuration.h which now has a "#include mechanics/rigidbot.h" to select all the mechanics of the kit, plus a "#define PREDEFINED_MECHANICS" in that rigidbot.h so that the same attributes in Configuration.h don't conflict with them. All Marlin users will still be editing Configuration.h for their needs, there is now just a way to select all the mechanical properties with a #include instead of several individual changes. |
|
Just swapping out mechanics is not a good solution. Different printers also come with different hot ends that need different PID and maximum temperature settings, many come with specific control panels, etc. You should really change out the whole configuration file (and possibly configuration_adv.h as well). |
|
Ok, I don't agree, but if this is a requirement for the rest of the pull request I'll remove the predefined mechanics. I think I've covered my perspective, but in case it isn't clear - I'm talking about "bundling" mechanical settings like build volume, acceleration, jerk, and endstops into predefined packages that correspond to mechanical kits that you can buy. Intentionally leaving things like hot ends, control panels, auto-z probes, etc. as separate settings (still in the main/only Configuration.h) precisely because people who buy a kit may easily replace those things. This mechanics/.h layout allows them to pick their kit, and separately pick their panel (RigidBot users seem to be preferring two at present), pick their hotend PIDs (from a selection in Configuration.h that I just added to for the RigidBot), and pick all the other features from there. If we instead have separate Configuration.h files for each kit (let's say 10 as oysteinkrog mentioned), each new feature in Marlin will require updating 11 Configuration.h files to address the new feature as on/off for each of them; vs. never changing the mechanics/.h files unless the new feature directly addresses mechanics. For example, on August 4, #0a8dc0e added two new options to Configuration.h and would have had to also update the 10 kits' Configuration.h files, and then #e7707ae which commented those new options out would have needed to remember updating the 10 kits' for that as well. Neither would touch mechanics/*.h files because it isn't changing mechanics. If that isn't persuasive I'll pull these portions out and leave Configuration.h mostly unchanged (commented out PID values, and commented out RigidPanel LCD support). I can also pull the automated build script example if you don't want it included. As far as complete Configuration.h files and Configuration_adv.h files, they can go in example_configurations/ just like what is already there for the MakiBox. Proving my prior point however, the example_configurations/makibox/Configuration.h file is old enough that it was never updated for thermal runaway protection settings. If MakiBox users are copying in this Configuration.h for their firmware builds they are likely unaware this support exists. |
|
I am glad for this conversation, it is exactly what I worried about when We have two problems which interact:
If we split up Configuration.h into smaller segments, it solves the I notice for MOTHERBOARD, we already have an appropriate #ifndef: #ifndef MOTHERBOARD I wonder if we could do that for all of the settings? Someone would have |
|
Thanks galexander1, I'm also grateful for this conversation. I'd like to be a long term Marlin contributor and I need to learn the preferences of the Marlin maintainers for things like this. In the case of wanting a file to "override" a default, I'm in favor of #undef/#define ... so you'd include Configuration.h, and then optionally include small files that overwrite certain properties. That can get confusing however, if it isn't super clear that the #define in the file you're editing might be altered by a choice elsewhere. I've only done #undef/#define within a single file where I feel it's very clear what should be expected. In fact, I've done this in this pullreq in languages.h for a few overwrites of "SD" with "USB" when a panel with "USB_LCD" is selected from Configuration.h. The alternative there would be an if/else on each of the related values, which I'm happy to discuss here as well. In an attempt to balance your problem # 1 and # 2 descriptions, I am advocating a Configuration.h that could be reduced down to "pick your motherboard", "pick your mechanics/kit" - or define custom mechanics, "pick your hotend", "pick your LCD", and finally "enable/disable a variety of miscellaneous features". That isn't quite a single-file fix for "premade 3d printers" but it's quite close, and it has advantages in maintainability. There is of course nothing stopping us from doing both of these approaches. Just because certain "kits" have bundled settings in mechanics/.h files, we could still have multiple whole Configuration.h files for "complete kits". I'm just saying I think those monolithic files will be left behind by most new commits, as all the ones in example_configurations/ have already (problem # 1). But maybe that's the concern of the folks trying to keep those files relevant. |
|
This is worth discussing, but if we decide to do it, it should be a separate pull request. |
example_configurations/rigidbot/ to capture some machine specifics
|
Ok, reverted Configuration.h to get rid of mechanics bundles entirely. Added an example_configurations/rigidbot/Configuration.h file to mirror the delta, scara, and makibox already there and capture some machine unique settings. The "automated build script" is now slightly broken because it would need to setup a lot more things before it could compile the various permutations of RigidBots - I can either leave it in as an example for people that want to experiment with their own, fix it now or later, or just delete it and maintain on my own. It's isolated from any "moving parts" of the Marlin code base. I think the only mildly controversial change left in this pull request would be the language.h changes to alter some strings to refer to "USB" instead of "SD". Specifically the use of #undef/#define as opposed to #if/#else ... if you want the latter I'd like to group any string constants that refer to "SD cards" into one block so I can make do with a single if/else block around them. Right now they're spread out within each language choice section. |
corrected the TEMP_SENSOR_ to the right value from -1 to 1
Correction from change of predefined mechanics back to this being a separate Configuration.h file in example_configurations/
|
Looking for input on what I can do to make this pull request more palatable. I noticed it was passed over in the recent merges. I'm relatively new to github, and completely new to Marlin development, so any advice is appreciated. Thanks, Greg |
|
Yes some advice/guidance would be good. I'm watching this discussion with interest as the current config setup somehow feels monolithic and cumbersome. At the end of the day it might be the only practical way to balance all competing concerns. Either way this is a good issue to thrash through and clear guidance from those who are gate keepers would be great for those of use learning what is and isn't seen as "good" and "done". Cheers! |
|
Hi Greg, I feel for your long wait on getting this reviewed and integrated, but keep your own fork alive in the meantime. This is often how people first get their custom firmware, as I can remember from pulling down various Delta versions of Marlin from GitHub for quite a while before it got integrated. I've been making small incremental contributions for a while, and at first the GitHub methodology was definitely confusing. But it makes sense now that I make liberal use of feature branches. As for what makes a good pull request, I'm sure you can find lots of tips, but here are a few that have helped me. As pull requests get stale it may be harder to get them in for review. It's a good idea to rebase your changes every so often to keep your fork fresh. Pull requests that can't be automatically merged are probably going to be slower than others. It may also help to find some RigidBot fans and get help reviewing and testing the code. Finally, package it up in such a way that it fits with the current Marlin paradigm, doesn't change too much of Configuration.h, and doesn't reinvent too much. I agree that Configuration.h and Configuration_adv.h are messes of many things in one place, and I'm sure many would like to see options they will never use placed into sub-files also. For example, you should only find #define DELTA in Configuration.h, and this would cause configuration/delta.h to be included. Then you would edit all the delta-specific options in delta.h. I'm not fond of the idea of having defaults and overrides with #ifdef and #undef, but I wouldn't mind seeing files like pins.h get split up into separate files, one for each of the individual machine ID numbers that are listed in Configuration.h. The "smartness" of pins.h is that it groups electronics with similar pins together, and so it is probably as concise as it can be. But it makes it impossible to do side-by-side comparison and diff, as we would be able to do if there were separate pins/pins##.h files for each unique ID. Things like this are hard to initiate and push through in an open source community project, especially when there are many push requests pending that modify Configuration.h and need it to remain fairly static. One way to work with this issue is to keep Configuration.h and Configuration_adv.h as they are, but start supplanting them with new configuration files. Over time more contributors will fetch the new configs and start working with them, and eventually the old files will be deprecated. |
|
Thank you Scott for the feedback. I had done a number of upstream merges early on, but until recently these changes were still clear for automerge, and with no input from the maintainers I kind of figured it was a dead end anyway. I'll take a look at the latest merge conflicts and resolve them for this pull request. Regarding testing, at this point many in the RigidBot community, about 2000 strong, are compiling directly off my fork (as well as another RB community member marvinstuart/Marlin who's added his own LCD controller setup support to my fork) and it's received all positive feedback so far in the g+ community. I have no idea how many people have built from my source, or just downloaded prebuilt .hex files I put up on our forums, but it is working for folks so far. Regarding the overall Marlin architecture, I agree it was a mistake to try and implement what I thought was a better organization at the outset, but all of that has been removed at this point. I'd also be happy to make any more changes maintainers want to see before accepting this, but they'd have to speak up first. I've been considering scrapping this pull request and using branches to break out the changes into even smaller chunks that might be accepted more quickly. Barring any input on what's needed for acceptance I'll probably go that route - a pull request for basic RigidBot controller support (just pins.h and example_configurations/rigidbot/), a pull request for the RigidPanel LCD controller, a pull request for a driver reset sequence the RigidBot uses, etc. and see if we can't get one of those in. Thanks again! Greg |
Conflicts: Marlin/language.h
|
Smaller pull requests are no doubt advantageous and easier to check. The big issue is the difficulty of getting bigger features tested and reviewed, and bringing over the tests and reviews from your own fork. But the popularity of your own fork speaks to its reliability, and that should count towards reviewing PRs. Collaborators can also look through your commits and see where bugs needed fixing, and of course look at your code carefully to see its quality. I don't know about other code-heads, but I am able to absorb and massage a lot of code at once, fingers flying over the meta keys. A major change across 7-10 files doesn't strike me as too daunting, thus my aim in reviewing code is to read through it and understand it fully, see if there are any obvious weaknesses, etc. But I do also know, as a developer, that if I can't get my hands on the machine and see the code in action, I feel incomplete as a reviewer. So another idea is for collaborators to reach out more to your users and ask them for a short review, and if they deem it worthy, and it doesn't cause other regressions or conflict with other configurations, consider it ready. New features get a period of probation, while longstanding features in the wild can be considered "stable." Notice we never get to tag it "perfect." |
|
I like the idea of supporting as much machines as possible... as for the config files my thinking is why not create a set of example configs that the user can use to overwrite the ones in the main folder.. in that way you stay clear of any issues... what is the general stand on merging this one? |
|
The trouble with that approach is that the example configs must be First, sometimes it breaks everything for people who are using one of the Second, even if it still "works", the new feature becomes invisible to You also run into problems for something like delta vs. corexy vs. There are a bunch of solutions to these problems -- even just having That said, that's how we've been doing it so far so I have nothing |
|
well i was just not sure, and have just become a collab i did not want to 2014-12-17 21:09 GMT+01:00 galexander1 [email protected]:
|
Conflicts: Marlin/cardreader.cpp
|
I just want to reemphasize that this pull request no longer includes any changes to the method of Configuration.h and Configuration_adv.h usage in Marlin. The original "predefined mechanics" block in Configuration.h that started this discussion was moved out to a mechanics/ subdirectory in commit a06578e, and was removed entirely in commit 417675c. As it currently stands there is an example_configurations/rigidbot/Configuration.h and RB users will be advised to copy this into place and edit. I dislike it for the reasons galexander1 outlined, but I'm more interested in seeing the RigidBot join the ranks of supported controllers and LCDs than trying to make a design change in Marlin as my first contribution. I would be happy to work on some feature branch implementations of different Configuration.h designs to propose, but I feel like it would go much smoother if there were a forum for conversations other than pull request comments. Is there a secret Marlin hideout where collaborators collaborate? Thanks again for your time. |
|
i only think there is the #reprap channel on irc freenode and then the comments here, when i got added as collab i was not told a special place for collabs to discuss changes... but it could be usefull since there are a lot of work hanging in the air |
|
I find these comment forums most convenient - everything in one place. But I'm all for live chat on IRC in those times when two or more of us might be coding or contemplating Marlin at the same time. |
|
@scott: I just meant if you can catch Erik van der Zalm on IRC then that's probably the easiest way to get added to the commit list. :) |
|
if not try send him a mail through his company.. https://www.ultimaker.com/pages/company/contact thou you might want to try both places... it took me an week to get hold of him until then let us other collabs know when you have something ready.... i'm just busy trying to flash the latest code on the OMC board i got added support for |
|
hehehe... GOD has long left this building :-D |
|
please rebase this on the latest v1 branch and submit a new pull request |
Invent-A-Part (https://www.inventapart.com/) has fulfilled Kickstarter rewards and begun selling their "RigidBot" 3d printer, with RAMPS-based controller, and a Marlin derivative firmware (forked from ErikZalm/Marlin #a964b3c; but not developed with git). IAP is apparently not interested in bringing their changes back into Marlin, but the community is very interested in having Marlin support the RigidBot so we can benefit from community developments like runaway heater protection, z autoleveling, etc. I've taken all the IAP changes made to #a964b3c and implemented compatible ones in the latest Marlin, with special attention to compartmentalizing and containing the code to not interfere with other platforms supported by Marlin. I've successfully compiled 20 permutations of motherboards, LCDs, and various configuration options, so I believe this code is ready for inclusion. A few highlights: