Question on Style

[jcassel]

@chunkysteveo, I am working on an update to handle when a printer is disconnected from OctoPrint.

Right now when your printer is disconnected from OctoPrint, the values in the printerStats are not updated and will hold whatever they held prior to the disconnect. This can be handled at the client side by purposefully inspecting the return code and then parsing out the return body. Not really so good. It should be possible to just look at the printerState that is part of the printerStats struct.

This update would make the assumption that the existing values in printerStats are untrustworthy and therefor should be set to values that reflect the idea that they are not the true current state of the Printer. Also the State text should reflect the unknonw state of the printer. In the case of a 409, there is a JSON object that we can get that text from. In the even of some other error code, it may not have the JSON error at the root and we have to handle that too.

The bool flags are pretty obvious as to what they should be set to in this situation. but the tool and bed values are not as clear. My inclination is to set them to a negative value to make it clear that they cant be real trusted values. This would give some level of obvious feedback to a client developer that did not for some reason look at the State text or the Response code. I would set the float types values to -999.

That being the case, I also see that there is some level of this thinking in how printJob is populated. It is populating floats with 0.0 and ints with 0. And it also sets any strings to empty ("") where there is an issue. Note that the the call to OctoPrint has a response for the Jobs info call returns null as values in the case where the printer is offline and the Printer State just does not return any data. Making it necessary to handle it logically different.

For me 0(Zero) is a real value and is not the same as null. For example. it is possible that the progress values could actually be zero. But the reality is that they are unknown in this state. We can't set the values of the int and float types to null but setting them to 0(zero) is not so good either.

In any change I make, I don't want to break anything that is a well established use case. Thus the ask as to how you would want an update related to this to be handled. Preference wise, setting the printerStats float data to -999 would be preferred but this would also make me want to update the values set in printJob to -999 where needed. The other option would be to set the printerStats to 0(zero) as it is done currently for printJob.

Thoughts?

This may help; the main part of the change to how the printerStats would be populated would look something like this:

   if(isResponseUnexpected){ //All values in the Stats Struct are no lonber good and should not be trusted. 

	//seems approprate to set these to true. Given the state is really unknown/error at his time.
	printerStats.printerStateclosedOrError = true; 
	printerStats.printerStateerror         = true;
	printerStats.printerStatefinishing     = false;
	printerStats.printerStateoperational   = false;
	printerStats.printerStatepaused        = false;
	printerStats.printerStatepausing       = false;
	printerStats.printerStatePrinting      = false;
	printerStats.printerStateready         = false;
	printerStats.printerStateresuming      = false;
	printerStats.printerStatesdReady       = false;
	printerStats.printerStateCancelling  = false;
	
	printerStats.printerBedTempActual = -999;
	printerStats.printerBedTempTarget = -999;
	printerStats.printerBedTempOffset = -999;
	printerStats.printerBedAvailable  = false;
	printerStats.printerTool0TempActual = -999;
	printerStats.printerTool0TempTarget = -999;
	printerStats.printerTool0TempOffset = -999;
	printerStats.printerTool0Available  = false;
	printerStats.printerTool1TempActual = -999;
	printerStats.printerTool1TempTarget = -999;
	printerStats.printerTool1TempOffset = -999;
	printerStats.printerTool1Available  = false;
	
	if(root.containsKey("error")){ //at least we have a well known response message here for state text.
		printerStats.printerState = (const char *)root["error"]; 
		return true;
	}else{//no idea what was returned. This is a very unlikely code path. Other than timeout.
		printerStats.printerState = "Unknown, see (httpErrorBody)";
		return false;
	}

[chunkysteveo]

I like your thinking @jcassel - going to read through this again to get it right in my head! But I get/like where you are coming from to tidy up the error states to be more obvious.

Let me read and think 👍

[jcassel]

I committed the changes I want to make in my Fork and tested. I am ready to create a pull request but I am not going to until I get a little feed back from you. I think this is the last change from me for a while as I have what I need to complete my project now. Let me know your thoughts when you have time.