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.

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

Added possibility for upload to Duet #1051

Closed
wants to merge 3 commits into from
Closed

Added possibility for upload to Duet #1051

wants to merge 3 commits into from

Conversation

@mloidl
Copy link
Contributor

mloidl commented Jul 13, 2018

This PR will add the possibility to send g-code to Duet and will fix #935 and #695

I've also did some changes in the background to make it easier to add additional printer hosts.

@x0rtrunks

This comment has been minimized.

Copy link

x0rtrunks commented Jul 15, 2018

This will be great. Thanks.

mloidl added 2 commits Jul 8, 2018
Further changes:
- Added new configuration option Host Type
- Added abstract base class for future printer hosts
- Moved location of upload dialog (also made it a little bit more configureable)
- added possibility to send file via postfield instead a new frame
@mloidl mloidl force-pushed the mloidl:duet branch from 9853693 to 4604581 Jul 17, 2018
@mloidl

This comment has been minimized.

Copy link
Contributor Author

mloidl commented Jul 17, 2018

Hi @bubnikv,

May you please have a look at the code?
I've tested the new g-code upload now several times with files up to 275Mb and it seems there are no problems at all.

Thank you.

@bubnikv

This comment has been minimized.

Copy link
Collaborator

bubnikv commented Jul 17, 2018

@vojtechkral vojtechkral self-assigned this Jul 18, 2018
@vojtechkral

This comment has been minimized.

Copy link
Contributor

vojtechkral commented Jul 18, 2018

Will look into it...

@vojtechkral

This comment has been minimized.

Copy link
Contributor

vojtechkral commented Aug 20, 2018

Hi. I've finally gotten around to review this PR. Sorry for the delay.

The PR looks very good in general, I just have a few small-ish requests:

  • Please update the code in PrintConfig.cpp`PrintConfigDef::handle_legacy() such that backwards compatibility is preserved, ie. that the octoprint_host, octoprint_cafile, and octoprint_apikey are converted to their updated counterparts.
  • In the same file, please also update the CLI option names
  • Please remove modifications to PrusaResearch.ini (we'll handle the removal of legacy options as part of the release process if needed, but they are empty anyway)

I might post some inline comments too as I proress with the review...

Thank you for the contribution!

* Added legacy code to preserve backwards compatibility
* renamed some cli option names  to better match option names
@mloidl

This comment has been minimized.

Copy link
Contributor Author

mloidl commented Aug 21, 2018

Hi @vojtechkral,

Yesterday i've uploaded a new commit which fixed the issues mentioned in your last comment.

@vojtechkral

This comment has been minimized.

Copy link
Contributor

vojtechkral commented Aug 21, 2018

@mloidl Thank you. I tested the code on our systems and had to perform a couple of bugfixes/refactoring.

There was a bug with the PrintHost class which was missing a virtual destructor, which is necessary when using child classes through a parent pointer. I also changed the postfield_add_file() as set_post_body(), as concatenaiting the content with a & wasn't really a valid operation. Additionally, the put_time function is not available on older versions of gcc which we need to use to support conservative Linux distros. I also made a bunch of smaller edits.

I hope you don't mind the changes. The code is in my vk-pr-1051 branch - would you please be so kind as to test whether it sill works for you after my changes? Unfortunatelly I don't have a Duet-equipped printer to test on.

@mloidl

This comment has been minimized.

Copy link
Contributor Author

mloidl commented Aug 21, 2018

@vojtechkral

Thank you very much for your review. I don't mind about the changes as my c++ is very rusty, so it can just getting better.

Nevertheless i had to do a small change in Duet::timestamp_str() to get it working again. The "time=" part of of the string got lost during refactoring.

I've added it again and pushed the commit to a new vk-pr-1051 branch on my fork. Not sure if i could push it directly to your branch.

After that single change the connection, file-upload and start print was working again.

@vojtechkral

This comment has been minimized.

Copy link
Contributor

vojtechkral commented Aug 22, 2018

@mloidl Ah yes, I forgot the time= ... I simplified the fix and merged the PR.

Thank you.

@mloidl

This comment has been minimized.

Copy link
Contributor Author

mloidl commented Aug 22, 2018

@vojtechkral Thank you. When i saw your change i was wondering why i did it the complicated way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.