Doodle3D / print3d

The application that runs on a Doodle3D WiFi box that communicates with printers.
www.doodle3d.com
GNU General Public License v2.0
13 stars 4 forks source link

Fix some compiler errors/warnings and add -p option to specify printer via the command line. #7

Closed oliv3r closed 10 years ago

oliv3r commented 10 years ago

Hey Doodle3D guys,

Let me start by introducing myself, I'm Olliver and I started working for Ultimaker. I was testing some things with print3d and found a few issues and patches those in this pull request. Also I added a new command line option to the server, that allows one to chose a printer from startup rather then depending on UCI. I haven't made the UCI dependancies fully optional yet, but removing/commenting them out does make everything work without UCI. Maybe more on that later.

Thanks,

Olliver

peteruithoven commented 10 years ago

Hi oliv3r, thanks for your contributions, they are greatly appreciated! We will check them at the end of the week. We are currently refactoring some stuff around the G-Code buffer (https://github.com/Doodle3D/doodle3d-client/issues/240 and https://github.com/Doodle3D/doodle3d-client/issues/226) so I hope there aren't to many conflicts. It would be great if you could open a issue before you start working on something, that would allow others to advice and to prevent overlapping contributions.

woutgg commented 10 years ago

Hey Olliver,

Thanks for your contributions! They're greatly appreciated. Regarding the option for to set the printer type however, I'd like two modifications: 1) move the type argument in the constructor to be last and give it an empty default; 2) the '-p' argument is currently required, it would be better if it was silently ignored when absent (or help shown when requested).

Btw, I'll be happy to make these changes myself if you have no objections to them.

oliv3r commented 10 years ago

I've updated the pull request with your recommendations, but a view points: 1) the parameter is now at the end, but it obviously can't be empty by default. On an empty printerName, the UCI variable gets read (or the hardcoded default, which could go away now). 2) The argument to -p is required, e.g. not using -p is okay, an empty printerName is sent to the contstructor which then asks UCI what to do. Using -p without an argument now silently fails and only getopt prints the error. It fails using an empty printerNam and hence UCI is being used. Using -p with help would be nice to have, but the API isn't implemented yet where we can get a list of supported printers (and their names). Once that piece of API is available, we can also check if the supplied printer name is correct, though I belive AbstractDriver does some form of checking there at the moment.

Olliver

woutgg commented 10 years ago

I've made several extra changes along the lines of the pull request including the possibility to list all available printer drivers/models. The driver factory looks up the supplied printer name in this list as well, and fails to instantiate a driver if the name is invalid. See: https://github.com/Doodle3D/print3d/commit/72b4ab55736edeefdec9a94fba21d05b291aa027