Begin support for Sd cards
ClosedPublic

Authored by rizzitello on Feb 4 2018, 3:48 PM.

Details

Summary

Added support for M20 - M30 including unit testing.
AtCore can now do some basic SD card commands.

  • Mount/UnMOunt (not on the gui)
  • List files on the card with clean names.
  • Delete most files on the card. Had issues deleting a file named "this is a file with spaces in the name" from my card
  • Print from the sd card. stock print, pause, stop , resume have been updated to suport sd opperations.

TODO:

  • SD card printing progress is not set up
  • Detect when sd prints end
  • Repetier Plugin
  • Marlin Plugin
  • Teacup Plugin (need to build Firmware)
  • Aprinter Plugin (need to build Firmware)
  • Sprinter Plugin (need to build Firmware)
  • Smoothie Plugin (need testers No smothieboard, #reprap says its just like repetier for sd ops.)

Future Improvements:

  • SD card prints when stoped do not currently return the current line or byte. This can be stored and used to resume prints later on .
  • Making new files. This is kinda weird since you open a file with M28 then all codes sent after this are written to the file until M29 is sent to end the writing.
Test Plan

This Tested. Built, Small jobs started and manipulated a few finished. It will need more testing on marlin marlins is kind of weird.

Tested on:

  • Repetier
  • Marlin

Diff Detail

Repository
R232 AtCore
Branch
sdCard
Lint
No Linters Available
Unit
No Unit Test Coverage
There are a very large number of changes, so older changes are hidden. Show Older Changes
tcanabrava added inline comments.Feb 5 2018, 1:40 PM
src/gcodecommands.cpp
542–559

I don't like those - too much code repetition. you can use
accepted

QString enumName= QMetaEnum::fromType<Class::Enum>().valueToKey(gcode);
return QObject::tr("ERROR! " + enumName + " It's obligatory to have an argument");

then remove the lot of duplication

rizzitello marked an inline comment as done.Feb 5 2018, 2:17 PM
rizzitello added inline comments.
src/atcore.cpp
64–67

Its only two bools so the gain from making them a bitmask would be negligible. I might remove the check for card mounted since it doesn't really matter one way or the other to the printer or the host . for instance if you request an filelist (m20) with no card mounted you just get an empty list..

src/atcore.h
209

if the mount fails it returns ok if the mount works it returns ok .. we have no way to return worked or didn't . aside from that atcore is not set in such a way that it can evaulate returns of commands it sends within the function it sends them in . All evalutation of returned messages happen in AtCore::newMessage and IFirmware::validateMessage.

215

same as above.

244

This is set by the firmware plugin . when messages are seen emited from the printer. This is now external because all Sd card support has been moved to firmware plugins.

mount and unmount send commands.. and they do not always provide the desired result. for instance if you have a printer with autodetect and you try to unmount it will unmount the card and instantly remount it. trying to send a mount command with a card inserted does nothing. most firmware support auto/mount and unmount by default for sd card readers.

256

Sd Card support is done buy firmware plugins . This is set by the firmware plugin when specific messages are recviced from the printer to indicate that is starting to send the file list and has finished sending the file list.

316

you can printer exactly as you were before the bool will default to false if not present and if present tells atcore to not start a print thread but instead start printing "fileName" from the sd card.

450

Because it is an action that some clients will want to provide to users. There is also a function to add files to the card Coming soon. M28/M29 are those codes.

src/gcodecommands.cpp
542–559

Thats a good idea , will do this after this diff.

rizzitello updated this revision to Diff 26585.Feb 5 2018, 2:20 PM
  • Tomaz suggestions
  • Enable pauseActions for sd Print pauses.
rizzitello updated this revision to Diff 26604.Feb 5 2018, 5:12 PM
  • Better detection on new file names on sd card
  • Mount checking adjusted in marlin plugin
rizzitello updated this revision to Diff 26793.Feb 8 2018, 9:23 PM
  • rebase to master
rizzitello added inline comments.Feb 10 2018, 2:51 PM
src/gcodecommands.cpp
542–559

please see D10331. Will be done in that style when its merged.

patrickelectric requested changes to this revision.Feb 11 2018, 6:20 PM
patrickelectric added inline comments.
src/atcore.cpp
391

We should remove the file only if the user want.

432

Why are you not changing the atcore state ?

525–529

almost all users use not sdcard to print, if put sdcard as a first option in this if, almost all users will miss the if and lost this processing time :)

689

this function should return the filelist ?

713

One thing is the client, other is atcore. The function need to make sense and not allow this kind of behaviour, this is a library and need to do what make sense, if the file does not exist we should qwarning a message about it and return.

src/gcodecommands.cpp
542–559

I agree with @tcanabrava.

testclient/mainwindow.cpp
616

Again, the most common case comes first in the if/else logic.

testclient/mainwindow.h
192

pb_ ? this is not a code standard name.

This revision now requires changes to proceed.Feb 11 2018, 6:20 PM
rizzitello updated this revision to Diff 26953.Feb 11 2018, 7:44 PM
rizzitello marked 13 inline comments as done.
  • More standard names
  • adjust AtCore::resume
src/atcore.cpp
391

This Clears the list of file we have stored on the sd card. So that our gui list is also cleared because we are no longer connected to a printer.

432

The state is changed in the stop command. The reason that its changed for non sd prints is because the thread will see the change then clean itself up . Sd Prints needs to be stoped a litte differently

525–529

We want to push the resume string we saved on pause anyway .

689

No that will the file list will be extracted from the retruns. AtCore has no way to pair sent commands with their returned strings.

src/gcodecommands.cpp
542–559

See D10436. It will resolve that and this will be done to be the same way.

testclient/mainwindow.cpp
616

This only happens if you hit the button to print from the sd card.

testclient/mainwindow.h
192

Newest update has more standard names

rizzitello updated this revision to Diff 26975.Feb 12 2018, 2:00 AM
  • rebase to master
rizzitello updated this revision to Diff 27111.Feb 13 2018, 9:52 PM
  • rebase with new GCodeCommands
  • clean up included GCodes to new GCodeCommand Style.
rizzitello updated this revision to Diff 27921.Feb 24 2018, 2:59 PM
rizzitello marked 2 inline comments as done.
  • Clean up stop , emergencyStop , add stopSdPrint(internal use only)
  • Do not attempt to stop sd card print on host disconnect
  • Space the cases in gcodecommands a little to its easier to read
rizzitello edited the summary of this revision. (Show Details)Feb 24 2018, 3:08 PM
rizzitello edited the summary of this revision. (Show Details)
rizzitello edited the test plan for this revision. (Show Details)
patrickelectric requested changes to this revision.Feb 24 2018, 5:13 PM
patrickelectric added inline comments.
src/atcore.cpp
67

make the 3 bools one after the other because of the memory alignment.

681

I don't know if this make sense, because isSdMounted can be false and mounted true, or otherwise.

src/plugins/marlinplugin.cpp
57

else ?

62

the test is not necessary, just core()->setSdMounted(true);

71

!total

78

total - temp.toLongLong();

81

Maybe, it's more safe to use progress >= 100

src/plugins/repetierplugin.cpp
71

same things in marlin

This revision now requires changes to proceed.Feb 24 2018, 5:13 PM
rizzitello marked 8 inline comments as done.Feb 24 2018, 9:10 PM
rizzitello added inline comments.
src/atcore.cpp
681

This is our one check that the state were setting it to is not the state it currently is . If they are equal we don't need to set them or do the emit.

rizzitello updated this revision to Diff 27957.Feb 24 2018, 9:11 PM
rizzitello marked an inline comment as done.
rizzitello edited the summary of this revision. (Show Details)
  • Patrick's suggestion
patrickelectric requested changes to this revision.Feb 25 2018, 12:06 AM
patrickelectric added inline comments.
src/atcore.cpp
313

Maybe we should create a messageHaveTemperatureData() in AtCore

525–529

I'm talking about branch prediction :)

667

if(delay)

719

?

src/plugins/marlinplugin.cpp
67

if(total)

This revision now requires changes to proceed.Feb 25 2018, 12:06 AM
rizzitello added inline comments.Feb 25 2018, 1:24 AM
src/atcore.cpp
719

There is no way to check this data that I am aware of. Docs say its almost always 0

rizzitello updated this revision to Diff 27975.Feb 25 2018, 1:53 AM
rizzitello marked 2 inline comments as done.
  • more patrick suggestions
rizzitello added inline comments.Feb 25 2018, 1:55 AM
src/atcore.cpp
313

yeah lets discuss that.

rizzitello updated this revision to Diff 28067.Feb 25 2018, 8:41 PM
  • Move Sd Related firmware stuff to protected make plugins friend classes
rizzitello updated this revision to Diff 28070.Feb 25 2018, 8:59 PM
  • Basic Sd Card support
patrickelectric accepted this revision.Feb 26 2018, 1:48 PM
patrickelectric added inline comments.
src/atcore.h
66

I'm a bit lost, what is the problem to use it with public functions ?

This revision is now accepted and ready to land.Feb 26 2018, 1:48 PM
rizzitello added inline comments.Feb 26 2018, 1:51 PM
src/atcore.h
66

Its not However this will prevent any use of the internal sd functions to be used for general use. Tomaz was worried about exposure and after thinking abut I decided it was safest to move all the functions we intend the plugins to use to protected so they are in one spot and can be used only by the friends classes

rizzitello edited the summary of this revision. (Show Details)Feb 27 2018, 9:58 PM
rizzitello added a subscriber: Atelier: AtCore.
laysrodrigues accepted this revision.Mar 1 2018, 10:22 PM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.