-
Notifications
You must be signed in to change notification settings - Fork 538
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Store average accelerometer readings in Object Model #590
base: 3.5-dev
Are you sure you want to change the base?
Conversation
Store the average value of each axis of the last accelerometer run in the Object Model
@@ -68,6 +68,8 @@ static volatile uint32_t numSamplesRequested; | |||
static uint8_t resolution = DefaultResolution; | |||
static uint8_t orientation = 20; // +Z -> +Z, +X -> +X | |||
static volatile uint8_t axesRequested; | |||
constexpr size_t NumAxis = 3; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this var name too generic?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NumAccelerometerAxes would be better
src/CAN/ExpansionManager.cpp
Outdated
{ | ||
{ | ||
nullptr, // no lock needed | ||
[] (const ObjectModel *self, const ObjectExplorationContext& context) noexcept -> size_t { return 3; }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you prefer array sizes are tracked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move NumAccelerometerAxes (renamed from NumAxis) into an existing common header file so that you can use that value instead of 3.
src/Platform/Platform.cpp
Outdated
// 2. Average readings from last accelerometer run | ||
{ | ||
nullptr, // no lock needed | ||
[] (const ObjectModel *self, const ObjectExplorationContext& context) noexcept -> size_t { return 3; }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you prefer array sizes are tracked?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See earlier comment
src/CAN/ExpansionManager.h
Outdated
@@ -26,6 +26,7 @@ struct ExpansionBoardData | |||
|
|||
const char *_ecv_array typeName; | |||
MinCurMax mcuTemp, vin, v12; | |||
float accelerometerLastRunAverages[3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How would you prefer array sizes are defined?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
@@ -87,6 +89,15 @@ static void AddLocalAccelerometerRun(unsigned int numDataPoints) noexcept | |||
reprap.BoardsUpdated(); | |||
} | |||
|
|||
static void AddLocalAccelerometerAverages(float averages[]) noexcept |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not add extra arguments to AddLocalAccelerometerRun instead of adding this function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will do the same thing for AddAccelerometerRun
as well
src/CAN/ExpansionManager.cpp
Outdated
{ | ||
{ | ||
nullptr, // no lock needed | ||
[] (const ObjectModel *self, const ObjectExplorationContext& context) noexcept -> size_t { return 3; }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Move NumAccelerometerAxes (renamed from NumAxis) into an existing common header file so that you can use that value instead of 3.
src/CAN/ExpansionManager.h
Outdated
@@ -26,6 +26,7 @@ struct ExpansionBoardData | |||
|
|||
const char *_ecv_array typeName; | |||
MinCurMax mcuTemp, vin, v12; | |||
float accelerometerLastRunAverages[3]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment
src/Platform/Platform.cpp
Outdated
// 2. Average readings from last accelerometer run | ||
{ | ||
nullptr, // no lock needed | ||
[] (const ObjectModel *self, const ObjectExplorationContext& context) noexcept -> size_t { return 3; }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See earlier comment
Add extra arguments to AddLocalAccelerometerRun instead of a separate function Move NumAccelerometerAxes to CANLib\RRF3Common.h
The second commit will require the changes in: Duet3D/CANlib#8 |
static void AddLocalAccelerometerAverages(float averages[]) noexcept | ||
{ | ||
for(unsigned int axis = 0;axis < NumAxis;++axis) | ||
for(unsigned int axis = 0;axis < NumAccelerometerAxes;++axis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggest use memcpyf here instead of a loop
@@ -479,12 +477,12 @@ GCodeResult Accelerometers::StartAccelerometer(GCodeBuffer& gb, const StringRef& | |||
# if SUPPORT_CAN_EXPANSION | |||
if (device.IsRemote()) | |||
{ | |||
reprap.GetExpansion().AddAccelerometerRun(device.boardAddress, 0); | |||
reprap.GetExpansion().AddAccelerometerRun(device.boardAddress, 0, {0}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to pass an array of the correct size here because the compiler doesn't know how many values are expected
} | ||
else | ||
# endif | ||
{ | ||
AddLocalAccelerometerRun(0); | ||
AddLocalAccelerometerRun(0, {0}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to pass an array of the correct size here because the compiler doesn't know how many values are expected, or add a new function AddFailedLocalAccelerometerRun that doesn't take the averages parameter
@@ -515,7 +513,7 @@ GCodeResult Accelerometers::StartAccelerometer(GCodeBuffer& gb, const StringRef& | |||
accelerometerFile->Close(); | |||
accelerometerFile = nullptr; | |||
MassStorage::Delete(accelerometerFileName.c_str(), false); | |||
reprap.GetExpansion().AddAccelerometerRun(device.boardAddress, 0); | |||
reprap.GetExpansion().AddAccelerometerRun(device.boardAddress, 0, {0}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to pass an array of the correct size here because the compiler doesn't know how many values are expected. Alternatively, add another function AddFailedAccelerometerRun that doesn't take the averages as a parameter and instead sets them to zero, since this appears to be done in several places.
@@ -599,15 +597,15 @@ void Accelerometers::ProcessReceivedData(CanAddress src, const CanMessageAcceler | |||
f->Truncate(); // truncate the file in case we didn't write all the preallocated space | |||
f->Close(); | |||
accelerometerFile = nullptr; | |||
reprap.GetExpansion().AddAccelerometerRun(src, 0); | |||
reprap.GetExpansion().AddAccelerometerRun(src, 0, {0}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment.
} | ||
else if (msg.axes != expectedRemoteAxes || msg.firstSampleNumber != expectedRemoteSampleNumber || src != expectedRemoteBoardAddress) | ||
{ | ||
f->Write("Received mismatched data\n"); | ||
f->Truncate(); // truncate the file in case we didn't write all the preallocated space | ||
f->Close(); | ||
accelerometerFile = nullptr; | ||
reprap.GetExpansion().AddAccelerometerRun(src, 0); | ||
reprap.GetExpansion().AddAccelerometerRun(src, 0, {0}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See previous comment.
|
||
// find the average value for each axis | ||
for (unsigned int axis = 0; axis < NumAxis; ++axis) | ||
for (unsigned int axis = 0; axis < NumAccelerometerAxes; ++axis) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Better to use a range-based for-loop here
src/CAN/ExpansionManager.cpp
Outdated
boards[address].accelerometerLastRunDataPoints = numDataPoints; | ||
++boards[address].accelerometerRuns; | ||
|
||
for(int32_t i = 0;i < NumAccelerometerAxes;++i) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we use memcpyf here?
Store the average value of each axis of the last accelerometer run in the Object Model