Differential D11370

More comments
ClosedPublic
Actions

Authored by rizzitello on Mar 15 2018, 9:11 PM.

Details

Reviewers
laysrodrigues
patrickelectric
tcanabrava
Commits
R232:ca4555e790a2: Various Fixes - AtCore::closeConnection: no longer try to unload a plugin if…
R232:d8497f69cc4e: - Remove AtCore::detectFirmware
R232:50607dca5ff0: AtCore::setIdleHold -> AtCore::disableMotors
R232:f78e2d5a0d3d: More comments
R232:ed95d7eed4de: Now with more comments
Summary
  • More Commented Code.
  • AtCore::closeConnection: no longer try to unload a plugin if one was not loaded.
  • AtCore Add missing space between functions
  • mainpage.md fix for lays kde mail
  • Remove AtCore::detectFirmware

Diff Detail

Repository
R232 AtCore
Branch
moreComments
Lint
No Linters Available
Unit
No Unit Test Coverage
rizzitello requested review of this revision.Mar 15 2018, 9:11 PM
rizzitello created this revision.
rizzitello updated this revision to Diff 29641.Mar 16 2018, 12:21 AM

Remove default_folder comment

rizzitello updated this revision to Diff 29643.Mar 16 2018, 12:25 AM
  • change a little wording
rizzitello updated this revision to Diff 29644.Mar 16 2018, 12:26 AM
  • Various Fixes
  • - Remove AtCore::detectFirmware
laysrodrigues requested changes to this revision.Mar 16 2018, 11:48 AM
laysrodrigues added inline comments.
src/atcore.cpp
422

really? I think that the signature of the method says enough

452

same here

693

Why this method doesnt have the name disableMotors or something like that? setIdleHold dont say nothing.

This revision now requires changes to proceed.Mar 16 2018, 11:48 AM
rizzitello marked 2 inline comments as done.Mar 16 2018, 11:57 AM
rizzitello added inline comments.
src/atcore.cpp
422

Yes I only put that becuse there is a stop for print job and a stop for sd prints. The code is very simple and does not require commenting.

452

Yes I only put that becuse there is a stop for print job and a stop for sd prints. The code is very simple and does not require commenting.

693

Disable Motors is done with a different Code this code is more commonly supported. There is no reason the function name can't be changed to AtCore::disableMotors.

laysrodrigues accepted this revision.Mar 16 2018, 12:04 PM
laysrodrigues added inline comments.
src/atcore.cpp
422

ok makes sense.

This revision is now accepted and ready to land.Mar 16 2018, 12:04 PM
rizzitello marked 6 inline comments as done.Mar 16 2018, 12:06 PM
rizzitello updated this revision to Diff 29695.Mar 16 2018, 7:58 PM
  • AtCore::setIdleHold -> AtCore::disableMotors
rizzitello marked an inline comment as done.Mar 16 2018, 8:02 PM
patrickelectric requested changes to this revision.Mar 16 2018, 8:02 PM
patrickelectric added inline comments.
src/atcore.cpp
76

Why the comments are not using the same indentation level ?

76

Register* ?

146

Use space after commas.

201

What ?

287

Why ?

338

//Printing from the sd card requires some M commands.

This revision now requires changes to proceed.Mar 16 2018, 8:02 PM
rizzitello marked 6 inline comments as done.Mar 16 2018, 8:22 PM
rizzitello added inline comments.
src/atcore.cpp
287

Why keep timer in memory if were not going to use it?
If the time is set to != it will make a new timer.

rizzitello updated this revision to Diff 29702.Mar 16 2018, 8:35 PM
rizzitello marked an inline comment as done.
  • Patrick's suggestions
  • Leaner AtCore::loadFirmwarePlugin
rizzitello marked an inline comment as done.Mar 16 2018, 8:35 PM
rizzitello updated this revision to Diff 29709.Mar 16 2018, 8:56 PM
  • Cleaner debug output for AtCore::loadFirmwarePlugin
patrickelectric requested changes to this revision.Mar 16 2018, 10:48 PM
patrickelectric added inline comments.
src/atcore.cpp
146

firmwares*, 'start', they firmware string name

287

because allocating and deallocating memory is worse than maintaining

347–349

The thread will handle the gcode file without freezing the library.
Besides that, the thread will take care of the printer feedback to only send the next command if necessary,
avoiding a serial buffer overflow.

365

//Append command to the commands queue.

446

throught*

522

Push*, coordinates.

700

boot.

This revision now requires changes to proceed.Mar 16 2018, 10:48 PM
rizzitello marked 7 inline comments as done.Mar 17 2018, 1:00 AM
rizzitello added inline comments.
src/atcore.cpp
446

through**

rizzitello updated this revision to Diff 29719.Mar 17 2018, 1:03 AM
rizzitello marked an inline comment as done.
  • Fix some spelling
  • AtCore::setSerialTimerInterval: Don't destroy timer if set to 0.
patrickelectric requested changes to this revision.Mar 17 2018, 1:21 PM
patrickelectric added inline comments.
src/atcore.cpp
349

is ready, avoiding buffer overflow in the printer.

365

I'm not a native english speaker, but this "on to the" is correct ? shouldn't it be only "to the" ?

377–378

What is clean print ? print is a variable ? I can't get what you are saying here only reading the comment. This need to be restructured.

523

This will be*

src/atcore.h
409

The brief already have the exactly same text, this line should be removed.

This revision now requires changes to proceed.Mar 17 2018, 1:21 PM
rizzitello updated this revision to Diff 29753.Mar 17 2018, 2:05 PM
rizzitello marked 3 inline comments as done.
  • rebase
rizzitello marked an inline comment as done.Mar 17 2018, 2:06 PM
patrickelectric accepted this revision.Mar 17 2018, 2:16 PM
This revision is now accepted and ready to land.Mar 17 2018, 2:16 PM
rizzitello closed this revision.Mar 17 2018, 10:21 PM

Revision Contents
Changeset List

PathPackages
Mdoc/mainpage.md (2 lines)
Msrc/atcore.h (22 lines)
Msrc/atcore.cpp (88 lines)
Mtestclient/mainwindow.cpp (5 lines)

Diff 29794

View Options

doc/mainpage.md

Loading...
View Options

src/atcore.h

Loading...
View Options

src/atcore.cpp

Loading...
View Options

testclient/mainwindow.cpp

Loading...