Create Machine Information Object
ClosedPublic

Authored by rizzitello on Apr 14 2019, 12:45 PM.

Details

Summary

We need an object to track our profiles this diff adds one.
MachineInfo allows for only one instance and should be easy to work with .
MachineInfo Major Changes:

  • Uses ini format to store our profiles in one of two paths. (transparent to clients )
    • appPath/profiles.ini
    • <QSettings user storage>/atcore/profiles.ini
  • Profile names are only groups
  • The bool for has heated bed has been removed; we can just check if bed max temp != 0
  • radius has been removed; Radius will be stored in the Max X value.
  • z_height_delta has been removed; Will now be stored in the Max Z value.

Widget is modeled after our profile dialog with a few differences,

  • it is a QWidget subclass
  • its using the MachineInfo object as its backend.
Test Plan

Update and run atcore it will prompt you to make profiles if you don't have any.

Diff Detail

Repository
R232 AtCore
Branch
machineInfo
Lint
No Linters Available
Unit
No Unit Test Coverage
Build Status
Buildable 11593
Build 11611: arc lint + arc unit
There are a very large number of changes, so older changes are hidden. Show Older Changes
  • Update testgui and printWidget for MachineInfo
rizzitello edited the summary of this revision. (Show Details)Apr 15 2019, 3:17 AM
rizzitello edited the test plan for this revision. (Show Details)
rizzitello updated this revision to Diff 56261.Apr 15 2019, 3:32 AM
rizzitello edited the test plan for this revision. (Show Details)
  • added more doxygen
patrickelectric requested changes to this revision.Apr 17 2019, 1:25 AM
patrickelectric added inline comments.
src/core/machineinfo.cpp
33

Use {foo}, to create a single indentation

46

This should be a pointer to be accessible in qml;

65

This values should be somewhere else no in the middle of the code. We should create a const standard profile in the class.

101

if does not contain return false, avoid indentation.

131

can you return childGRoups directly ? why m_settings is a pointer ?

src/core/machineinfo.h
38

this should be a namespace or a enum class

39

Start the first one as zero, we may add something before name

60

When you are a QObject copy is not allowed, this looks unnecessary.

61

why this is public ?

This revision now requires changes to proceed.Apr 17 2019, 1:25 AM
rizzitello updated this revision to Diff 56408.Apr 17 2019, 2:48 AM
rizzitello marked 9 inline comments as done.
  • Address Patrick's conserns
rizzitello added inline comments.Apr 17 2019, 2:48 AM
src/core/machineinfo.cpp
46

I was thinking it would be used like this in the main cpp file

qmlEngine.rootContext()->setContextProperty("machineInfo", MachineInfo::getInstance());
rizzitello updated this revision to Diff 56409.Apr 17 2019, 3:02 AM
  • clean up some more leading comma for multiline calls
  • Use ini for windows always and native otherwise
patrickelectric requested changes to this revision.Apr 19 2019, 12:37 AM
patrickelectric added inline comments.
src/core/machineinfo.cpp
58

Why not a QMap<MachineInfo::KEY, QPair<QString, QVariant>> ?

68

this is not necessary, you just need to save the settings inside a profiles configuration list.

69

does not need to be a pointer.

src/core/machineinfo.h
51

NICE

This revision now requires changes to proceed.Apr 19 2019, 12:37 AM
rizzitello marked 3 inline comments as done.
  • most of patricks suggestions
  • ability to rename / copy profiles
rizzitello added inline comments.Apr 19 2019, 11:02 PM
src/core/machineinfo.cpp
58

made a small struct with keyString and defaultValue Instaed but a similar idea.

laysrodrigues accepted this revision.Fri, Apr 26, 2:45 PM

It looks ok for me

patrickelectric requested changes to this revision.Fri, Apr 26, 3:51 PM
patrickelectric added inline comments.
src/core/machineinfo.h
140

is there a need to be a pointer ?

src/widgets/profilemanager.cpp
57

else is unnecessary use return

138

Is this the upside-down number of the beast ?
use std::numeric_limits<T>::max()

140

well, this is huge, you should move to a function and stay with only min number of lines inside lambda functions.

270

if empty return

313

get by copy if you are doing a copy.

This revision now requires changes to proceed.Fri, Apr 26, 3:51 PM
rizzitello updated this revision to Diff 57079.Sat, Apr 27, 2:47 AM
rizzitello marked 5 inline comments as done.

Patricks Suggestions

laysrodrigues accepted this revision.Thu, May 2, 6:22 PM
patrickelectric requested changes to this revision.Sat, May 4, 10:33 PM
patrickelectric added inline comments.
src/core/machineinfo.cpp
46

you just need to register as a singleton, please use a pointer.
google for qmlregistersingletontype

59

is it possible to do this population with a for loop ?

77

This line is huge.

83

qCWarning missing

87

qCWarning missing

105

qCWarning missing

109

qCWarning missing

113

qCWarning missing

142

qCWarning missing

163

is it possible to do that with a for ?

src/core/machineinfo.h
49

comma in the end

57

get is not usually used in our api and Qt api, I think this function should be instance() or self().

140
This revision now requires changes to proceed.Sat, May 4, 10:33 PM
rizzitello updated this revision to Diff 57580.Sun, May 5, 2:50 AM
rizzitello marked 13 inline comments as done.
  • More cleanup
rizzitello updated this revision to Diff 57612.Sun, May 5, 8:48 PM
  • Pointer to object
  • more thread safe
rizzitello added inline comments.Sun, May 5, 8:50 PM
src/core/machineinfo.cpp
87

This is triggered a bunch of times. You see it with the profile manager If your starting profile shares any values with what the defaults are set to one warning per instance of that same if you change between profiles and they any profile data in common you will get more of these in the log.

src/core/machineinfo.h
140

It is a pointer because At creation time we can automaticly set the paths for settings without any additional code. If you make this a regular object you have to do this via QSettings::setPath and the docs for that say "Warning: This function doesn't affect existing QSettings objects.". So now we can't make it a class member at all and instead in our code we have to have some kind of settings* return for access across the object. I've tried several ways to make it just a QSettings object but it does not even build correctly when its not used as a pointer.

patrickelectric requested changes to this revision.Mon, May 6, 1:52 PM
patrickelectric added inline comments.
src/core/machineinfo.cpp
46

You should return a pointer, but you don't need to create the object in the heap.

static MachineInfo m;
return &m;

Do not forget to set the ownership to cpp

This revision now requires changes to proceed.Mon, May 6, 1:52 PM
rizzitello updated this revision to Diff 57644.Mon, May 6, 2:45 PM
  • Better use for qml
rizzitello marked 3 inline comments as done.Mon, May 6, 2:46 PM
rizzitello marked 2 inline comments as done.
patrickelectric accepted this revision.Mon, May 6, 2:47 PM
This revision is now accepted and ready to land.Mon, May 6, 2:47 PM
tcanabrava accepted this revision.Mon, May 6, 2:51 PM
rizzitello updated this revision to Diff 57646.Mon, May 6, 2:54 PM
  • pre merge Rebase
rizzitello closed this revision.Mon, May 6, 2:54 PM