Skip to content
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

MakerBot plugin #5

Merged
merged 91 commits into from
Oct 31, 2023
Merged

MakerBot plugin #5

merged 91 commits into from
Oct 31, 2023

Conversation

casperlamboo
Copy link
Contributor

CURA-10561

jellespijker and others added 15 commits October 16, 2023 10:28
Contribute to CURA-10561
Included a new io.cpp within the DULCIFICUM_SRC in CMakeLists.txt. Implemented functionality to read files from disk, specifically focused on Gcode files for now. This utility will aid in processing Gcode commands for the Dulcificum project. Future enhancements will include the ability to read other file formats.

Contributes to CURA-10561
Contribute to CURA-10561
Added multiple headers to include/dulcificum/gcode/ast for AST handling using variants and templates. Also added function to parse gcode into AST. These changes allow transforming gcode to AST which can be used for further analysis or translation to other formats. Added example usage to the main translator application.

Contribute to CURA-10561
This commit refactors the Gcode Abstract Syntax Tree (AST) classes to include translation functionality. It introduces a map of creator functions that allows for dynamic creation of different types of nodes, improving code reusability and maintainability. This change is necessary to ensure that each different Gcode command could be parsed and handled independently, promoting code flexibility and easier debugging. Now each node can execute its own logic when the operator() is called. The capability of line translation is demonstrated using G0 command as an example.

In addition, the uncommented code in apps/translator_main.cpp has been commented out. Overall, this commit aims to provide a more structured and flexible approach to parsing and handling different Gcode commands.

Contribute to CURA-010561
The AST structure has been fundamentally changed from a standard vector to a multimap to support a tree-like structure, which is more representative of the GCode's structure and improves its traversal. A DFS function has been added to traverse the modified AST. A root node is returned from the parser and is used as the starting point for the DFS. This root node and visited nodes are managed by shared pointers for memory safety. The G0 operator overload has now been finalized. Changes in code also necessitated changes in the creators - they now return shared pointers. Lastly, the BaseEntry class has been introduced as parent of Entry class for polymorphism.

Contributes to CURA-10561
Which will act mediator language between the different languages.

Add new files and refactor code to implement AST-based parsing and translation of GCode

New source and header files added for Abstract Syntax Tree (AST) based system to handle GCode translation. This change helps improve code maintainability by unifying parsing and translation functionality. The changes also include modification of existing files, classes, and function signatures to accommodate and support the new system.

Notably, new classes for AST nodes, such as Translate, ast_t and node_t have been designed. For parsing GCode, a factory class is created that maps GCode instructions to corresponding AST nodes. For translating GCode to Proto Path, a translate function is added.

Also, to make it more readable and aligned, the code has been refactored, primarily renaming and relocating components for better structure. Furthermore, file and class headers were renamed to match current usage.

Contribute to CURA-10561
A StringViewToDouble conversion function is added. With help of this function, improved the Gcode parsing to extract more data for each Gcode command. Now it correctly identifies and extracts all relevant command data for each command line, including optional parameters, and stores them in corresponding classes' member variables. More specifically, the changes were made in classes representing G0, G1, m104, and other Gcode commands. Additionally, for each updated class a std::optional field was added for each optional command parameter which can appear in actual Gcode command, allowing to check if certain parameter was set in the given Gcode command.

Contribute to CURA-10561
Renamed file src/gcode/ast/operators.cpp to src/gcode/ast/entries.cpp to better reflect its contents and role. Also, modified parse error messages to include the line that failed to parse. This change is meant to improve debugging capabilities. All instances of the filename in other scripts, like CMakeLists.txt, have been updated accordingly.

Contributes to CURA-10561
@github-actions
Copy link

github-actions bot commented Oct 23, 2023

Unit Test Results

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites  - 1   0 💤 ±0 
0 files    - 1   0 ±0 

Results for commit d755576. ± Comparison against base commit a806ed4.

♻️ This comment has been updated with latest results.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/2)

* P = Printing acceleration. Used for moves that include extrusion
* T = Travel acceleration. Used for moves that include no extrusion
*/
class M204 : public Entry<R"(M204((?:\sP(?<P>\d+(?:\.\d+)?))|(?:\sT(?<T>\d+(?:\.\d+)?))|(?:\sS(?<S>\d+(?:\.\d+)?)))*$)">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-virtual-class-destructor ⚠️
destructor of M204 is public and non-virtual

* Z = Z max jerk (units/s)
* E = E max jerk (units/s)
*/
class M205 : public Entry<R"(M205((?:\sX(?<X>-?\d+(?:\.\d+)?))|(?:\sY(?<Y>-?\d+(?:\.\d+)?))|(?:\sZ(?<Z>-?\d+(?:\.\d+)?))|(?:\sE(?<E>-?\d+(?:\.\d+)?)))*$)">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-virtual-class-destructor ⚠️
destructor of M205 is public and non-virtual

* /brief Set a new target bed temperature (non-blocking).
* S = Target temperature
*/
class M140 : public Entry<R"(M140((?:\sS(?<S>\d+(?:\.\d+)?)))*$)">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-virtual-class-destructor ⚠️
destructor of M140 is public and non-virtual

* R = Target temperature (wait for cooling or heating).
* S = Target temperature (wait only when heating)
*/
class M190 : public Entry<R"(M190((?:\sS(?<S>\d+(?:\.\d+)?))|(?:\sR(?<R>\d+(?:\.\d+)?)))*$)">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-virtual-class-destructor ⚠️
destructor of M190 is public and non-virtual

* /brief The layer index
* L = index
*/
class Layer : public Entry<R"(;LAYER:(?<L>-?\d+))">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-virtual-class-destructor ⚠️
destructor of Layer is public and non-virtual

* E = An absolute or relative coordinate on the E axis (in current units).
* F = The maximum movement rate of the move between the start and end point.
*/
class G1 : public Entry<R"(G1((?:\sX(?<X>-?\d+(?:\.\d+)?))|(?:\sY(?<Y>-?\d+(?:\.\d+)?))|(?:\sZ(?<Z>-?\d+(?:\.\d+)?))|(?:\sE(?<E>-?\d+(?:\.\d+)?))|(?:\sF(?<F>-?\d+(?:\.\d+)?)))*$)">

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-virtual-class-destructor ⚠️
destructor of G1 is public and non-virtual

using node_t = std::variant<Translate>;
using ast_t = std::vector<node_t>;

node_t factory(const auto& ast)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ misc-unused-parameters ⚠️
parameter ast is unused

Suggested change
node_t factory(const auto& ast)
node_t factory(const auto& /*ast*/)

template<utils::CharRangeLiteral Axis>
struct Coordinate : Magnitude
{
static constexpr std::string_view axis{ Axis.value };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for class constant axis

Suggested change
static constexpr std::string_view axis{ Axis.value };
static constexpr std::string_view AXIS{ Axis.value };

namespace dulcificum::utils
{

double StringViewToDouble(std::string_view sv);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for function StringViewToDouble

Suggested change
double StringViewToDouble(std::string_view sv);
double stringViewToDouble(std::string_view sv);

namespace dulcificum::utils
{

double StringViewToDouble(std::string_view sv);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-length ⚠️
parameter name sv is too short, expected at least 3 characters

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (2/2)

gcode::ast::ast_t ast;

size_t index{ 0 };
while (std::getline(stream, line))

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ altera-id-dependent-backward-branch ⚠️
backward branch (while loop) is ID-dependent due to variable reference to stream and may cause performance degradation

for (auto& node : gcode)
{
std::visit(
[](auto& n)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-length ⚠️
parameter name n is too short, expected at least 3 characters


namespace dulcificum::utils
{
double StringViewToDouble(std::string_view sv)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-length ⚠️
parameter name sv is too short, expected at least 3 characters

@@ -0,0 +1,45 @@
#ifndef INCLUDE_DULCIFICUM_GCODE_AST_ACCELERATION_H
#define INCLUDE_DULCIFICUM_GCODE_AST_ACCELERATION_H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AST?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abstract Syntax Tree, Although this tree basically consist of a single trunk

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh ahahaha that wouldn't have occured to me. TAC (Threeletter Acronyms Confuse) :P

casperlamboo and others added 11 commits October 24, 2023 15:13
G92 changes the origin, and thus only changes the state since no concept of a changing origin is known to the internal command list

CURA-10561
gave run time errors

CURA-10561
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

@@ -82,7 +83,8 @@ nlohmann::json getCommandParameters(const botcmd::Command& cmd)
{
const auto dcmd = static_cast<const botcmd::SetTemperature&>(cmd);
jparams[k_key_str::index] = dcmd.index;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

@@ -104,6 +106,16 @@ nlohmann::json getCommandParameters(const botcmd::Command& cmd)
jparams[k_key_str::seconds] = dcmd.seconds;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

if (cmd.type == botcmd::CommandType::kLayerChange)
{
const auto dcmd = static_cast<const botcmd::LayerChange&>(cmd);
auto comment = botcmd::Comment{};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

jellespijker and others added 6 commits October 29, 2023 16:02
Contributes to CURA-10561
Replace pointer dereferencing with std::optional::value() for safer code. Previously code used pointer dereferencing to access optional command parameters which can lead to undefined behavior when the optional is not set, now it uses std::optional::value() which throws a defined exception (`bad_optional_access`) in that case.

Contributes to CURA-10561
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

// TODO: Unknown
};
void to_proto_path(const ast::G0_G1& command);
void to_proto_path(const ast::G4& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::G4& command);
void toProtoPath(const ast::G0_G1& command);

};
void to_proto_path(const ast::G0_G1& command);
void to_proto_path(const ast::G4& command);
void to_proto_path(const ast::M104& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::M104& command);
void toProtoPath(const ast::G4& command);

void to_proto_path(const ast::G0_G1& command);
void to_proto_path(const ast::G4& command);
void to_proto_path(const ast::M104& command);
void to_proto_path(const ast::M106& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::M106& command);
void toProtoPath(const ast::M104& command);

void to_proto_path(const ast::G4& command);
void to_proto_path(const ast::M104& command);
void to_proto_path(const ast::M106& command);
void to_proto_path(const ast::M107& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::M107& command);
void toProtoPath(const ast::M106& command);

void to_proto_path(const ast::M104& command);
void to_proto_path(const ast::M106& command);
void to_proto_path(const ast::M107& command);
void to_proto_path(const ast::M109& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::M109& command);
void toProtoPath(const ast::M107& command);

include/dulcificum/state.h Show resolved Hide resolved
bool is_retracted{ false };

int64_t layer{ 0 };
std::optional<int64_t> layer_index_offset{ std::nullopt };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-diagnostic-error ⚠️
no template named optional in namespace std

bool is_retracted{ false };

int64_t layer{ 0 };
std::optional<int64_t> layer_index_offset{ std::nullopt };

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-diagnostic-error ⚠️
no member named nullopt in namespace std

}
else if (command.P)
{
delay->seconds = command.P.value() * 1000.0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-avoid-magic-numbers ⚠️
1000.0 is a magic number; consider replacing it with a named constant

@@ -82,7 +84,8 @@ nlohmann::json getCommandParameters(const botcmd::Command& cmd)
{
const auto dcmd = static_cast<const botcmd::SetTemperature&>(cmd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

@@ -2,8 +2,10 @@

#include "dulcificum/miracle_jtp/mgjtp_mappings_json_key_to_str.h"

#include <fmt/format.h>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ llvm-include-order ⚠️
#includes are not sorted properly

Suggested change
#include <fmt/format.h>
#include <fmt/format.h>

@@ -2,8 +2,10 @@

#include "dulcificum/miracle_jtp/mgjtp_mappings_json_key_to_str.h"

#include <fmt/format.h>
#include <range/v3/view/remove_if.hpp>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ llvm-include-order ⚠️
#includes are not sorted properly

Suggested change
#include <range/v3/view/remove_if.hpp>
#include <range/v3/view/remove_if.hpp>

@@ -2,8 +2,10 @@

#include "dulcificum/miracle_jtp/mgjtp_mappings_json_key_to_str.h"

#include <fmt/format.h>
#include <range/v3/view/remove_if.hpp>
#include <range/v3/view/zip.hpp>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ llvm-include-order ⚠️
#includes are not sorted properly

Suggested change
#include <range/v3/view/zip.hpp>
#include <range/v3/view/zip.hpp>

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

void to_proto_path(const ast::Layer& command);
void to_proto_path(const ast::Comment& command);
void to_proto_path(const ast::T& command);
void to_proto_path(const ast::InitialTemperatureExtruder& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::InitialTemperatureExtruder& command);
void toProtoPath(const ast::InitialTemperatureExtruder& command);

jellespijker and others added 2 commits October 29, 2023 16:57
Changed the naming convention of CommandType enum items from `kItem` to `Item` in both `mgjtp_mappings_json_key_to_str.h` and `command_types.h`. Also updated all usage of these enums in various files. The change was made to improve the code readability, as the "k" prefix was considered redundant and potentially confusing for some developers.

Contributes to CURA-10561
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

// TODO: InitialTemperatureBuildPlate
// TODO: BuildVolumeTemperature
// TODO: Unknown
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
};
void toProtoPath(const ast::G0_G1& command);

// TODO: BuildVolumeTemperature
// TODO: Unknown
};
void to_proto_path(const ast::G0_G1& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::G0_G1& command);
void toProtoPath(const ast::G4& command);

// TODO: Unknown
};
void to_proto_path(const ast::G0_G1& command);
void to_proto_path(const ast::G4& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::G4& command);
void toProtoPath(const ast::M104& command);

};
void to_proto_path(const ast::G0_G1& command);
void to_proto_path(const ast::G4& command);
void to_proto_path(const ast::M104& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::M104& command);
void toProtoPath(const ast::M106& command);

void to_proto_path(const ast::G0_G1& command);
void to_proto_path(const ast::G4& command);
void to_proto_path(const ast::M104& command);
void to_proto_path(const ast::M106& command);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-identifier-naming ⚠️
invalid case style for method to_proto_path

Suggested change
void to_proto_path(const ast::M106& command);
void toProtoPath(const ast::M107& command);

{
const auto com = static_cast<const botcmd::Comment&>(cmd);
jparams[k_key_str::comment] = com.comment;
return jparams;
}
if (cmd.type == botcmd::CommandType::kActiveFanDuty)
if (cmd.type == botcmd::CommandType::ActiveFanDuty)
{
const auto dut = static_cast<const botcmd::FanDuty&>(cmd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

{
const auto dut = static_cast<const botcmd::FanDuty&>(cmd);
jparams[k_key_str::index] = dut.index;
jparams[k_key_str::value] = dut.duty;
return jparams;
}
if (cmd.type == botcmd::CommandType::kActiveFanEnable)
if (cmd.type == botcmd::CommandType::ActiveFanEnable)
{
const auto fan = static_cast<const botcmd::FanToggle&>(cmd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

return jparams;
}
if (cmd.type == botcmd::CommandType::kWaitForTemperature)
if (cmd.type == botcmd::CommandType::WaitForTemperature)
{
const auto dcmd = static_cast<const botcmd::WaitForTemperature&>(cmd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

{
const auto dcmd = static_cast<const botcmd::WaitForTemperature&>(cmd);
jparams[k_key_str::index] = dcmd.index;
return jparams;
}
if (cmd.type == botcmd::CommandType::kChangeTool)
if (cmd.type == botcmd::CommandType::ChangeTool)
{
const auto dcmd = static_cast<const botcmd::ChangeTool&>(cmd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

{
const auto dcmd = static_cast<const botcmd::ChangeTool&>(cmd);
jparams = zipListsIgnoreNan(k_key_str::k_param_point_names, dcmd.position);
jparams[k_key_str::index] = dcmd.index;
return jparams;
}
if (cmd.type == botcmd::CommandType::kDelay)
if (cmd.type == botcmd::CommandType::Delay)
{
const auto dcmd = static_cast<const botcmd::Delay&>(cmd);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-pro-type-static-cast-downcast ⚠️
do not use static_cast to downcast from a base to a derived class

casperlamboo and others added 2 commits October 29, 2023 17:20
Contributes to CURA-10561
@casperlamboo casperlamboo changed the title Gcode parser MakerBot plugin Oct 30, 2023
jellespijker and others added 2 commits October 30, 2023 11:00
If no P index is given 0 is assumed

Contributes to CURA-10561
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/1)


// set fan speed
const auto set_fan_speed = std::make_shared<botcmd::FanDuty>();
set_fan_speed->duty = command.S.has_value() ? command.S.value() : 255;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-avoid-magic-numbers ⚠️
255 is a magic number; consider replacing it with a named constant

@jellespijker jellespijker marked this pull request as ready for review October 30, 2023 11:25
jellespijker and others added 2 commits October 31, 2023 13:58
Contributes to CURA-10561

Co-authored-by: c.lamboo <[email protected]>
Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ clang-tidy found issue(s) with the introduced code (1/1)

}
}

void VisitCommand::to_proto_path(const gcode::ast::G0_G1& command)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ readability-function-cognitive-complexity ⚠️
function to_proto_path has cognitive complexity of 34 (threshold 25)

state.active_tool == 1 && command.E.has_value() ? delta_e : 0.0,
};
// gcode is in mm / min, bot cmd uses mm / sec
move->feedrate = state.F[state.active_tool] / 60.0;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ cppcoreguidelines-avoid-magic-numbers ⚠️
60.0 is a magic number; consider replacing it with a named constant

Copy link
Contributor

@wawanbreton wawanbreton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • A lot of code-style remarks
  • Some optimization proposals
  • A few possible bugs
    Hope this helps !

apps/translator_main.cpp Show resolved Hide resolved
conanfile.py Show resolved Hide resolved
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does AST stand for ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Abstract Syntax Tree


[[nodiscard]] std::string GCode2Miracle_JTP(const std::string& content)
{
auto gcode_ast = dulcificum::gcode::parse(content);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How are errors/warnings reported ?


[[nodiscard]] std::string GCode2Miracle_JTP(const std::string& content)
{
auto gcode_ast = dulcificum::gcode::parse(content);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please do not use auto when the type is not explicit

src/gcode/gcode_to_command.cpp Show resolved Hide resolved
state.active_tool == 0 && command.E.has_value() ? delta_e : 0.0,
state.active_tool == 1 && command.E.has_value() ? delta_e : 0.0,
};
// gcode is in mm / min, bot cmd uses mm / sec
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bot cmd should use standard units as much as possible.. Technically, seconds is the standard unit time and mm/s is widely used for 3D printing so that should be used instead.

src/gcode/gcode_to_command.cpp Show resolved Hide resolved
else if (delta_e == 0)
{
move->tags.emplace_back(botcmd::Tag::TravelMove);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an empty line here as these are big if/else if

src/gcode/gcode_to_command.cpp Outdated Show resolved Hide resolved
@casperlamboo casperlamboo merged commit 5da7dd6 into main Oct 31, 2023
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants