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.

Sign up for GitHub

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

Jump to bottom

Option to skip griffin header validation. #44

Open
wants to merge 3 commits into
base: master/s-line
Choose a base branch
from
Open

Option to skip griffin header validation. #44

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
dde7dfc
Option to skip griffin header validation.
rburema Mar 16, 2023
0a5d864
More readable according to reviewer.
rburema Mar 21, 2023
fb361dd
Static variable must have class name.
rburema Mar 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 29 additions & 17 deletions Charon/filetypes/GCodeFile.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ class GCodeFile(FileInterface):
mime_type = "text/x-gcode"

MaximumHeaderLength = 100
SkipHeaderValidation = False
Copy link
Contributor

Choose a reason for hiding this comment

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

In general, double negatives are something you may want to avoid. Better to introduce a setting ValidateHeader with a default of True.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the general sentiment. However, the (extra) negative here is meant to imply that this is not standard behaviour.

I'm perfectly fine with changing it if there are any strong feelings about this though.

Copy link
Contributor

Choose a reason for hiding this comment

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

No particularly strong feelings, if it's something you've considered already. I do see your point :)


def __init__(self) -> None:
self.__stream = None # type: Optional[IO[bytes]]
Expand Down Expand Up @@ -68,9 +69,13 @@ def parseHeader(stream: IO[bytes], *, prefix: str = "") -> Dict[str, Any]:

flavor = metadata.get("flavor", None)
if flavor == "Griffin":
if metadata["header_version"] != "0.1":
if metadata["header_version"] != "0.1" and not SkipHeaderValidation:
rburema marked this conversation as resolved.
Show resolved Hide resolved
raise InvalidHeaderException("Unsupported Griffin header version: {0}".format(metadata["header_version"]))
GCodeFile.__validateGriffinHeader(metadata)
try:
GCodeFile.__validateGriffinHeader(metadata)
except InvalidHeaderException as ex:
if not GCodeFile.SkipHeaderValidation:
raise ex
Comment on lines +74 to +78
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not a simple

if not GCodeFile.SkipHeaderValidation:
    GCodeFile.__validateGriffinHeader(metadata)

? Isn't that more straightforward than the whole try..except? Or maybe I'm missing something?

Copy link
Member

Choose a reason for hiding this comment

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

We do this because apparently, the validate function also parses part of the header.

Rewriting it so that it first validates, then parses would be a bit out of the scope of this small change.

This is also why we moved the parts in the validation around.

Copy link
Contributor

Choose a reason for hiding this comment

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

We do this because apparently, the validate function also parses part of the header.

With that, I presume you mean that __validateGriffinHeader() also returns or modifies some kind of state? I'm probably "looking with my nose", as we'd say, but the only thing I see is the function checking things and raising exceptions. It has neither a return value, nor does it modify (global) state.

So I don't think your concern is actually valid? (Otherwise I'd agree that you'd want to limit the scope of these changes)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that's what I meant when I wrote that :-)

... now that I'm re-checking the function, I can see what you say. Strange, I could've sworn that something was saved in that function. I remember trying it without. -- Perhaps I did something wrong when I originally tried that.

If we're not both wrong, we could indeed try to resolve it the way you originally suggested here.

GCodeFile.__cleanGriffinHeader(metadata)
elif flavor == "UltiGCode":
metadata["machine_type"] = "ultimaker2"
Expand Down Expand Up @@ -195,7 +200,25 @@ def __validateGriffinHeader(metadata: Dict[str, Any]) -> None:
raise InvalidHeaderException("PRINT.SIZE.MIN.[x,y,z] must be set. Ensure all three are defined.")
if not GCodeFile.__isAvailable(metadata, ["print", "size", "max", ["x", "y", "z"]]):
raise InvalidHeaderException("PRINT.SIZE.MAX.[x,y,z] must be set. Ensure all three are defined.")


# Validate extruder train, part I
for index in range(0, 15):
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the increment from the original 10 to 15? And could you perhaps make this a named constant while you're at it?

Copy link
Member

Choose a reason for hiding this comment

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

What the user sees in the front-end is a max of 8 extruders. In the engine (and some of the frontend code as well) we assume max 16 extruders in code. I'm not sure where '10' came from.

Yes, a constant would be good.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks, that's clear! A named constant with a small comment explaining that bit would indeed be helpful for the future. Thanks 🙏🏻

index_str = str(index)
if GCodeFile.__isAvailable(metadata, ["extruder_train", index_str]):

if not GCodeFile.__isAvailable(metadata, ["extruder_train", index_str, "nozzle", "diameter"]) or \
not isAPositiveNumber(metadata["extruder_train"][index_str]["nozzle"]["diameter"]):
raise InvalidHeaderException(
"extruder_train.{}.nozzle.diameter must be defined and be a positive real".format(index))

if not GCodeFile.__isAvailable(metadata, ["extruder_train", index_str, "initial_temperature"]) or \
not isAPositiveNumber(metadata["extruder_train"][index_str]["initial_temperature"]):
raise InvalidHeaderException(
"extruder_train.{}.initial_temperature must be defined and positive".format(index))

# Put those closest to 'optional' interpretation last,
# so most of the meta-data is still processed when SkipHeaderValidation is on.

# Validate print time
print_time = -1

Expand All @@ -208,27 +231,16 @@ def __validateGriffinHeader(metadata: Dict[str, Any]) -> None:

if print_time < 0:
raise InvalidHeaderException("Print Time should be a positive integer")
# Validate extruder train
for index in range(0, 10):

# Validate extruder train, part II
for index in range(0, 15):
index_str = str(index)
if GCodeFile.__isAvailable(metadata, ["extruder_train", index_str]):

if not GCodeFile.__isAvailable(metadata, ["extruder_train", index_str, "nozzle", "diameter"]) or \
not isAPositiveNumber(metadata["extruder_train"][index_str]["nozzle"]["diameter"]):
raise InvalidHeaderException(
"extruder_train.{}.nozzle.diameter must be defined and be a positive real".format(index))

if not GCodeFile.__isAvailable(metadata, ["extruder_train", index_str, "material", "volume_used"]) or \
not isAPositiveNumber(metadata["extruder_train"][index_str]["material"]["volume_used"]):
raise InvalidHeaderException(
"extruder_train.{}.material.volume_used must be defined and positive".format(index))

if not GCodeFile.__isAvailable(metadata, ["extruder_train", index_str, "initial_temperature"]) or \
not isAPositiveNumber(metadata["extruder_train"][index_str]["initial_temperature"]):
raise InvalidHeaderException(
"extruder_train.{}.initial_temperature must be defined and positive".format(index))

def getStream(self, virtual_path: str) -> IO[bytes]:
assert self.__stream is not None

Expand Down