Cleanup Gcodecommands
AbandonedPublic

Authored by rizzitello on Feb 5 2018, 8:06 PM.

Details

Summary
  • Remove GcodeCommands

Diff Detail

Repository
R232 AtCore
Branch
gcodeCleanup
Lint
No Linters Available
Unit
No Unit Test Coverage
rizzitello requested review of this revision.Feb 5 2018, 8:06 PM
rizzitello created this revision.
rizzitello retitled this revision from Summary: Use a consistant error message for when a gcode requires at least one argument. Update GcodeTests User ternary conditionals to make toCommand more readable to Cleanup Gcodecommands.Feb 5 2018, 8:07 PM
rizzitello edited the summary of this revision. (Show Details)
laysrodrigues accepted this revision.Feb 6 2018, 12:34 AM
laysrodrigues added inline comments.
src/gcodecommands.cpp
0

Loved this inline if. Congrats for the improve! =D

This revision is now accepted and ready to land.Feb 6 2018, 12:34 AM
patrickelectric requested changes to this revision.Feb 7 2018, 6:38 PM
patrickelectric added inline comments.
src/gcodecommands.cpp
1
msg = value1.isEmpty() ? GCode::commandRequiresArgument(gcode) : QStringLiteral("M109 S%1").arg(value1);

Print the name of gcode command with the message.

1

Maybe we should use cooked literals

This revision now requires changes to proceed.Feb 7 2018, 6:38 PM
rizzitello updated this revision to Diff 26741.Feb 8 2018, 2:08 AM
  • Remove Use of gcodecommands
  • Remove GcodeCommands
rizzitello updated this revision to Diff 26791.Feb 8 2018, 9:14 PM
  • update base
  • Remove Use of gcodecommands
  • Remove GcodeCommands

why ??

  • Remove Use of gcodecommands
  • Remove GcodeCommands

why ??

  • Less code to maintain
  • Lots of code for little gain
  • Code is slightly more readabe in atcore with only QStringLiteral("G28") instead of GCode::toCommand(GCode::G28));
  • GCode::toString was used 5 times. Strings returned by this might not be accurate for your firmware. This class contained lots of strings for the translators that we never used.

Maybe this could be used in atelier to help users make gcodes ? ?

rizzitello edited the summary of this revision. (Show Details)Feb 8 2018, 11:39 PM
laysrodrigues requested changes to this revision.Feb 9 2018, 1:14 AM
laysrodrigues added inline comments.
src/atcore.cpp
409

Here too

416

Missing use of QStringLiteral?
Because on the other calls you use it.

This revision now requires changes to proceed.Feb 9 2018, 1:14 AM
rizzitello marked 2 inline comments as done.Feb 9 2018, 1:20 AM
rizzitello added inline comments.
src/atcore.cpp
409

this bypasses the pushCommand so that the queue is skipped and the serialLayer version of pushCommand takes a QByteArray

416

Same as above

  • Remove Use of gcodecommands
  • Remove GcodeCommands

why ??

  • Less code to maintain
  • Lots of code for little gain
  • Code is slightly more readabe in atcore with only QStringLiteral("G28") instead of GCode::toCommand(GCode::G28));
  • GCode::toString was used 5 times. Strings returned by this might not be accurate for your firmware. This class contained lots of strings for the translators that we never used. Maybe this could be used in atelier to help users make gcodes ? ?

I disagree, this is a abstraction for the commands and dialects, reverting this class will reflect in a big problem on the future. That's my point of view.

rizzitello marked 2 inline comments as done.EditedFeb 9 2018, 1:42 AM

I disagree, this is a abstraction for the commands and dialects, reverting this class will reflect in a big problem on the future. That's my point of view.

There is no dialect stuff here thats all in the firmware plugins. I thought about some of the cases where we can have options with things like 'I' and other args that are not standard for most codes. We can if/when we support those options we can have a more dynamic way to add the arguments for the few gcodes that could require them.

The best thing is to replace the gcode class to more generic classes. But
remove this abstraction is a kickback, we can use this class to provide a
lot of important information to the graphic interface, gcode editor, gcode
information and etc.. besides that, isn't hard to maintain what is standard
protocol, only small things change between this main class.
If the gcode class isn't helpfully we need to reformulate it and no remove.

we can use this class to provide a
lot of important information to the graphic interface, gcode editor, gcode
information and etc..

This is why i suggested it would be more useful to a client of atcore. I don't think it belongs in atcore or is needed for any of the opperations that atcore does now or will be doing.

I feel the same way as patrick on this one - I understand that you want less code but I feel that missing the debug information as missing parameters for a command to be a good thing

Ok I'll go back to my previous version of this diff isn't tereany opps

rizzitello abandoned this revision.Feb 10 2018, 5:37 PM

We will improve the GCodeCommands class instead