Improved Example

[vicimikec]

Improved the custom_m_codes.py script to include handling a S parameter. This includes evaluating a Meta Variable passed in the S parameter.

Also fixed the call to resolve_code on line 56. It was being passed MessageType.Warn. It should be MessageType.Warning.

[chrishamm]

Thanks! I've adopted that change from Warn to Warning.

Yet I'm not quite happy with the way you evaluate expressions. Instead of checking if the string param starts with {, you could just query the code parameter's is_expression value which should be set by the constructor. Otherwise (although unlikely) you could pass "{foo" to a code as a parameter and the check would fail.

[vicimikec]

That does sound like a better solution. Unfortunately I modified my code on my test system and is_expression does not seem to be getting set. Here is debug output:

root@Duetoko:~# python3.7 custom_m_codes.py
send: {"mode":"Intercept","version":12,"InterceptionMode":"Pre","Channels":["HTTP","Telnet","File","USB","Aux","Trigger","Queue","LCD","SBC","Daemon","Aux2","Autopause","Unknown"],"Filters":["M1234","M5678","M7722"],"PriorityCodes":false}
recv: {"success":true}
recv: {"sourceConnection":193,"result":null,"type":"M","channel":"HTTP","lineNumber":3,"indent":0,"keyword":0,"keywordArgument":null,"majorNumber":7722,"minorNumber":null,"flags":2072,"comment":null,"filePosition":null,"length":21,"parameters":[{"letter":"S","value":"{var.minutes}","isString":false}],"command":"Code"}

Looking in code_parameter.py I am not sure if this would work correctly:

self.is_expression = self.string_value.startswith("{}") and self.string_value.endswith("}")

pretty sure it should be:

self.is_expression = self.string_value.startswith("{") and self.string_value.endswith("}")

even then I would think the parameter would be set in the JSON.

[LoicGRENON]

Looking in code_parameter.py I am not sure if this would work correctly:

self.is_expression = self.string_value.startswith("{}") and self.string_value.endswith("}")

pretty sure it should be:

self.is_expression = self.string_value.startswith("{") and self.string_value.endswith("}")

even then I would this the parameter would be set in the JSON.

Good catch !
There is the same mistake at line 63 : https://github.com/Duet3D/dsf-python/blob/main/src/dsf/commands/code_parameter.py#L63

On a side note about your initial commit, you can remove the parenthesis around the if expressions, they are useless in python.
As well as the test for None, you can use if variable is None.
Then, I'm a bit concerned about passing expression to subprocess.run directly from gcode without any sanity check.

[vicimikec]

I switched:
value.startswith("{}") and value.endswith("}")
to
value.startswith("{") and value.endswith("}")
on lines 44 and 63 in code_parameters.py and is_expression is now being set properly. I realized the JSON I am seeing in the debug output is coming from DSF before dsf-python processes it which would be why is_expression is not included.

I also switched my example to use is_expression and made some formatting changes.

[vicimikec]

Thanks! I've adopted that change from Warn to Warning.

Yet I'm not quite happy with the way you evaluate expressions. Instead of checking if the string param starts with {, you could just query the code parameter's is_expression value which should be set by the constructor. Otherwise (although unlikely) you could pass "{foo" to a code as a parameter and the check would fail.

@chrishamm Is there anything else you need to get this code merged?

[LoicGRENON]

@chrishamm Is there anything else you need to get this code merged?

Please look at my comments above

[vicimikec]

Sorry. I did not see the part about the sanity checks before subprocess.run(). I will add a check to make sure that it is passing specifically an integer. Are there any other checks you would like me to add?

[LoicGRENON]

You shouldn't have to use perform_command. You should rather use command_connection.evaluate_expression directly as evaluate_expression is a method of BaseCommandConnection. See https://github.com/vicimikec/dsf-python/blob/main/src/dsf/connections/base_command_connection.py#L56

It may also be possible to pass the intercept connection as parameter of eval_expr function so you won't have to open another connection to send that command.

[LoicGRENON]

result will be undefined if there is an exception triggered above. You have to define result = 1 before the try...finallyclause.

[vicimikec]

Hmm. Problem there is that 1 is a valid return value even though the function failed. Would it be better to set this to False or None? I will be honest Python is not my strongest programming language so I am not sure what the convention of indicating a failure in these instances is.

[LoicGRENON]

You can set it to None. If something fails, an exception will be thrown anyway

[LoicGRENON]

Sorry, I didn't see I needed to "submit" these review changes ...
They were displaying on the discussion above but it seems they were only displaying for me ... (I had to check in incognito mode to figure it out).