Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 9 commits into from
May 1, 2014
Merged

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

merged 9 commits into from
May 1, 2014

Conversation

oliv3r
Copy link
Contributor

@oliv3r oliv3r commented Apr 14, 2014

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

Olliver Schinagl added 5 commits April 10, 2014 13:13
build-local contains bash-isms. depend on bash
7d362b0 
The *buntu's come with dash as the default shell and build-local.sh
fails to build because of it as it requires bash.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Git should not track *~ and .*.swp files
1873c0e 
*~ and .*.swp files are backup files used by editors. Git should always
ignore them.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Add missing algorithm.h define for std::count
3d56599 
std::count requires algorithm.h to be included or the compiler gives an
error.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Fix compiler warnings/errors due to wrong types
d17f895 
The compiler warns and errors due to the mis-use of unsigned vs signed
and size_t vs int in the unittests.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Correct the usage of %zi vs %d in printf
97dbd27 
currentLine, totalLines, bufferedLines are all ints and thus do not
require the 'z' length modifier that is required for size_t.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
@peteruithoven
Copy link
Member

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 (Doodle3D/doodle3d-client#240 and Doodle3D/doodle3d-client#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
Copy link
Contributor

woutgg commented May 1, 2014

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.

Olliver Schinagl added 4 commits May 1, 2014 11:45
Add a -p option to choose which printer the print server controls
aeb427f 
When not using UCI to obtain the configuration settings, a dummy
variable forces a makerbot (or whatever printer) as default. Using -p
one of the available printers can be configured via the commandline.
When -p is not supplied, uci (or the dummy var) is used instead.

Missing still is to print the list of available printers when using -p
help or an empty -p. Since there is a function in the 'TODO' area to
print a list of printers, this item is also TODO.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Require a device ID for several actions, fixes segfault
f3a57bb 
THe AT_GET_TEST an AT_GET_PROGRESS actions require a device ID when,
also when called from -g. If this is not done, calling these actions via
-g causes segfaults.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Fix segfault when doing serial communication.
62928f6 
The serial module does not account for the \0 character when mallocing.
Request 1 extra byte of memory to account for \0.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
Use getopt's api as intended, store the return value in an int, not char
7b01f60 
getopt returns an integer which is -1 when there is no more options left to be parsed.
Currently this result is stored in a char, which by definition is undefined whether
it is signed or unsigned. On ARM it may be unsigned and thus -1 never happens.

Signed-off-by: Olliver Schinagl <o.schinagl@ultimaker.com>
@oliv3r
Copy link
Contributor Author

oliv3r commented May 1, 2014

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 woutgg merged commit 7b01f60 into Doodle3D:develop May 1, 2014
@woutgg
Copy link
Contributor

woutgg commented May 1, 2014

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: 72b4ab5

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@oliv3r @peteruithoven @woutgg