Differential D10787

Part 1
ClosedPublic
Actions

Authored by laysrodrigues on Feb 24 2018, 5:17 AM.

Details

Reviewers
rizzitello
tcanabrava
patrickelectric
Commits
R231:dd4a0c79832c: First commit of AtCore Instance Widget
Summary

Create a new widget to be base of any atcore instance.
Most of the basic features are implemented. All of them
were based on previous code. So is still missing basic stuff

TODO:

  • Printing Flow
  • Connection Flox

Signed-off-by: Lays Rodrigues <laysrodriguessilva@gmail.com>

Diff Detail

Repository
R231 Atelier
Branch
part1
Lint
No Linters Available
Unit
No Unit Test Coverage
laysrodrigues requested review of this revision.Feb 24 2018, 5:17 AM
laysrodrigues created this revision.
laysrodrigues retitled this revision from First commit of AtCore Instance Widget to Part 1.Feb 24 2018, 5:19 AM
rizzitello added inline comments.Feb 24 2018, 1:11 PM
src/widgets/atcoreinstancewidget.h
52

The Normal UI Comment. This class has a very simple ui that is just a layout.

src/widgets/atcoreinstancewidget.ui
69

Becareful with using gridlayouts they can hard to adjust for right to left languages.

rizzitello added inline comments.Feb 24 2018, 1:13 PM
src/widgets/atcoreinstancewidget.cpp
59

I don't see much point in allowing the hiding these at this point.

rizzitello requested changes to this revision.Feb 24 2018, 1:55 PM
rizzitello added inline comments.
src/widgets/atcoreinstancewidget.cpp
29

Not needed.

149

Todo: post Pause String in profile, use it here

159

M18 is not supported on all firmwares. please see AtCore::setIdleHold
Use: m_core.setIdleHold(0); Since M84 is supported by all firmwares and does the same with optional delay untill motors are active again.

169

enableControls (true);
Connecting is Connected with unknown firmware. can still use the controls at this point. printing is the only questionable thing in this state.

176

do not enable controls here.

183

enableControls (false);

284

return (AtCore::state() != DISCONNECTED )

src/widgets/atcoreinstancewidget.h
67

Not Needed.

68

This function is not needed.

This revision now requires changes to proceed.Feb 24 2018, 1:55 PM
patrickelectric requested changes to this revision.Feb 24 2018, 4:19 PM
patrickelectric added inline comments.
src/widgets/atcoreinstancewidget.cpp
65

you don't need the *.

101

reference.

143

m_core.print(fileName.toLocalFile());

164

static

207

Add default

214

static msg

253

const ref

260

Where are you sending the command, and how are you check if it's valid here ?

266

Use atcore command.

laysrodrigues marked 11 inline comments as done.Feb 24 2018, 11:21 PM
laysrodrigues added inline comments.
src/widgets/atcoreinstancewidget.cpp
149

There's no PauseString setup on profile. Need to be added.

169

I don't think that we should enable a connection if the firmware is unknown, it's not safe. And at least in Atelier I want safety first.

176

Why?

207

What would be the default? just default: break?
I don't think that is mandatory use default on swich/cases.

260

This method is based on AtCore Gui:
https://github.com/KDE/atcore/blob/master/testclient/mainwindow.cpp#L224

It uses a connect to check the pushedCommand.
connect(m_core.serial(), &SerialLayer::pushedCommand, this, &AtCoreInstanceWidget::checkPushedCommands);

laysrodrigues updated this revision to Diff 27966.Feb 24 2018, 11:22 PM

Fixes suggested by Sith and Patrick

patrickelectric requested changes to this revision.Feb 24 2018, 11:59 PM
patrickelectric added inline comments.
src/widgets/atcoreinstancewidget.cpp
43

reference

80

const ref serialPort and profiles

101

You need to use reference and not copy inside the lambda declaration [&]

133

lost comment

207

Not mandatory, but this can avoid future problems in AtCore state changes, you can do something like qWarning/qError/qFatal or something like in default.

260

I believe that should remove multiple spaces, trailing spaces, is inside ascii table and not utf-8.

284

and != ERRORSTATE

This revision now requires changes to proceed.Feb 24 2018, 11:59 PM
laysrodrigues updated this revision to Diff 27969.Feb 25 2018, 12:49 AM
laysrodrigues marked 3 inline comments as done.

More fixes and add method to setup the connections

laysrodrigues added a comment.Feb 25 2018, 12:50 AM

Damn it. Idk why the Profile changes came here =/

src/widgets/atcoreinstancewidget.cpp
133

It's because when the event happens on AxisControl it sends a QLatin1Char as argument, so I need to change the AxisControl to support AtCore::AXES... This will take a while.

laysrodrigues abandoned this revision.Feb 25 2018, 12:52 AM
laysrodrigues reclaimed this revision.
laysrodrigues updated this revision to Diff 27972.Feb 25 2018, 12:54 AM

Use reference

rizzitello requested changes to this revision.Feb 25 2018, 2:02 AM
rizzitello added inline comments.
src/widgets/atcoreinstancewidget.cpp
169

Then do it when idle. Hopefully no one ever trys to use a control with an unknown firmware.

284

ERRORSTATE is still connected to a device @patrickelectric . The only state where you are not connected is DISCONNECTED.

This revision now requires changes to proceed.Feb 25 2018, 2:02 AM
laysrodrigues added inline comments.Feb 25 2018, 2:16 AM
src/widgets/atcoreinstancewidget.cpp
169

It is already done there.

284

indeed.

rizzitello added a comment.Feb 25 2018, 3:14 AM

Think thats the last of them

src/widgets/atcoreinstancewidget.cpp
91

remove this line

137

Extra empty line

280

extra line

rizzitello added inline comments.Feb 25 2018, 3:19 AM
src/widgets/atcoreinstancewidget.cpp
266

Can we connect these objects and remove this function?

laysrodrigues added inline comments.Feb 25 2018, 12:36 PM
src/widgets/atcoreinstancewidget.cpp
266

No, because it needs to adapt the AxisControl to support AtCore::AXES. and that it's kind complex(I tried last night...) I can remove the function and dont have any support for axes movement now.
Also, I'm still deciding if we will use Tomaz Pie or the widget that I made... I'm trying to think in a third option.

rizzitello added inline comments.Feb 25 2018, 12:41 PM
src/widgets/atcoreinstancewidget.cpp
266

Im reviewing this code you have proposed right now. does it work like it is here? if so a connect should be trivial.. if not why commit code that doesn't work

Also, I'm still deciding if we will use Tomaz Pie or the widget that I made... I'm trying to think in a third option.

Then why are you even changing anything here now?

laysrodrigues marked 3 inline comments as done.Feb 25 2018, 1:19 PM
laysrodrigues added inline comments.
src/widgets/atcoreinstancewidget.cpp
266

So I will get back the previous code, without using atcore move command.

laysrodrigues updated this revision to Diff 28028.Feb 25 2018, 1:28 PM

Fix axis method
Not using atcore.move method for now, because it will need
more changes on AxisControl.

rizzitello added inline comments.Feb 25 2018, 6:54 PM
src/widgets/atcoreinstancewidget.h
31

Can we get the docs started?

laysrodrigues added inline comments.Feb 25 2018, 7:02 PM
src/widgets/atcoreinstancewidget.h
31

Idk We can add to the backlog. Because this will impact all the application, so I think that we can start it after we merge refactor into master.

rizzitello added inline comments.Feb 25 2018, 7:22 PM
src/widgets/atcoreinstancewidget.h
31

Its Best to Document as you write.

rizzitello accepted this revision.Feb 26 2018, 12:57 AM
patrickelectric accepted this revision.Feb 26 2018, 1:41 PM
patrickelectric added inline comments.
src/widgets/atcoreinstancewidget.cpp
284

What errorstate stands for ?

This revision is now accepted and ready to land.Feb 26 2018, 1:41 PM
rizzitello added inline comments.Feb 26 2018, 1:47 PM
src/widgets/atcoreinstancewidget.cpp
284

ERRORSTATE is when the printer has had a malformed command sent it will return ERROR: then it wants you to resend the previous command .. we have a task open to handle those T6732.

Revision Contents
Changeset List

Diff 28028

View Options

src/widgets/CMakeLists.txt

Loading...
View Options

src/widgets/atcoreinstancewidget.h

Loading...
View Options

src/widgets/atcoreinstancewidget.cpp

Loading...
View Options

src/widgets/atcoreinstancewidget.ui

Loading...