Marlin: SHOW_TEMP_ADC_VALUES errors

[repetier]

Just got a report from a user that with

define SHOW_TEMP_ADC_VALUES

the temperatures do not get read correctly. User said it was 1.1RC6 and his screenshot showed

00:43:17.873 : ok T:24.8 /0.0 B:25.2 /0.0 B@:0 @:0 ADC B:25.2C->977 T0:24.8C->977
while my tests with RC7 showed
00:43:17.873 : ok T:24.8 /0.0 B:25.2 /0.0 B@:0 @:0 ADC B:25.2C->977 T :24.8C->977
The space after last T is a 0 - byte and was supposed to be a 0 for extruder number. Guess iterator is now a char so it is not converted into a number. So that is the least error you should fix.

Also I'm not so hot about adding C->977 after it. This breaks past host parsers as that is part from B: or T0: and belongs to the result and the result is no number any more, which is why host then shows 0 temperature instead. Not sure which other hosts will have similar problems. It only works if you parse next number neglecting non number chars afterwards, but as I saw at my example this assumption must not be true.

[repetier]

Sidenote: The 0 value also appears in normal temperature output without SHOW_TEMP_ADC_VALUES. Line should now look like this
SERIAL_PROTOCOL((int)e);

[Blue-Marlin]

Likely the types of e should be the same.

#if HOTENDS == 1
  #define HOTEND_LOOP() const uint8_t e = 0;
  #define HOTEND_INDEX  0
  #define EXTRUDER_IDX  0
#else
  #define HOTEND_LOOP() for (int8_t e = 0; e < HOTENDS; e++)
  #define HOTEND_INDEX  e
  #define EXTRUDER_IDX  active_extruder
#endif

[thinkyhead]

Also I'm not so hot about adding C->977 after it.

This has been in there for a while, apparently. It exists in 1.0.2-1, for example. I'm not sure where it came from: https://github.com/MarlinFirmware/Marlin/blob/1.0.x/Marlin/Marlin_main.cpp#L2433-L2448

So, first, I can add a space before the C-> to fix the numeric parsing. Will it also help to change that odd -> to a colon? And will older hosts be able to read the temperature values correctly with the extra bit (which is the "raw" temperature) appended?

As for the other part — displaying incorrectly — possibly it's because I added a serial-output handler for char that displays the character instead of the integer. This should be fixable by casting to int.

Attn: @MarlinFirmware/host-software-team

[repetier]

Am 28.07.16 um 08:19 schrieb Scott Lahteine:

Also I'm not so hot about adding C->977 after it.

This has been in there for a while, apparently. It exists in 1.0.2-1,
for example. I'm not sure where it came from:
https://github.com/MarlinFirmware/Marlin/blob/1.0.x/Marlin/Marlin_main.cpp#L2433-L2448

So, first, I can add a space before the |C->| to fix the numeric
parsing. Will it also help to change that odd |->| to a colon? And
will older hosts be able to read the temperature values correctly with
the extra bit appended?

Yes, a space is the termination of a argument parameter. Something
starting with C-> would never be asked for so it is just part of ignored
response, that is ok.

As for the other part — displaying incorrectly — possibly it's because
I added a serial-output handler for |char| that displays the character
instead of the integer. This should be fixable by casting to |int|.

Exactly what I did to fix it. This problem might also appear in other
places where you go over extruders as your iteration variable seems to
be a char and now does not get autoconverted to int any more.

[thinkyhead]

This problem might also appear in other places

I'll see if I can constrain it so serial_echopair_P(const char* s_P, char v) only treats actual char as char and treats uint8_t and int8_t as int. If I can't force that, then I will relent and get rid of the char version of serial_echopair_P. I'm sure it's only being used for debugging output anyway.

[repetier]

Just add a version for uint8_t that calls with int cast.

[foosel]

What has already been said plus: Could someone please link me to some documentation or explain what all those individual fields are supposed to represent and why some of them are duplicated?

[thinkyhead]

explain what all those individual fields are supposed to represent and why some of them are duplicated?

Here's the order of output which I glean by reading the code:

• T:nnn /nnn - Temperature and target of the target extruder
• B:nnn /nnn - Temperature and target of the bed

If there are multiple hotends, they are inserted at this point:

• T0:nnn /nnn
• T1:nnn /nnn

Next comes the power output:

• B@:nnn - Bed power output (0-127) or watts (see below)
• @:nnn - Hotend(s) power output (0-127) or watts (see below)

With SHOW_TEMP_ADC_VALUES enabled it (currently) appends:

• ADC - Indicate the start of ADC values (4 spaces before "ADC")
• B:nnnC - Current temperature of bed in celsius
• ->nnn - Raw bed temperature (divided by OVERSAMPLENR)
• T0:nnnC (etc.) - Celsius temperature of hotend
• ->nnn - Raw hotend temperature (divided by OVERSAMPLENR)

So, with only the base options and a single hotend M105 says:

ok T:123 /180 B:20 /70 B@:123 @:123

If you set EXTRUDER_WATTS and/or BED_WATTS then it will replace the "heater power" with wattage output, appending a W to the value.

ok T:123 /180 B:20 /70 B@:9W @:3W

With SHOW_TEMP_ADC_VALUES the output (currently) looks like:

ok T:24.8 /0.0 B:25.2 /0.0 B@:0 @:0    ADC B:25.2C->977 T0:24.8C->977

[thinkyhead]

To fix the current issue with the least disruption:

ok T:24.8 /0.0 B:25.2 /0.0 B@:0 @:0    ADC B:977 T0:977

Or I could change the output so the raw (ADC) temperature is shown inline:

ok T:24.8 /0.0 (977) B:25.2 /0.0 (977) B@:0 @:0

Or, with multiple hotends…

ok T:24.8 /0.0 (977) B:25.2 /0.0 (977) T0:24.8 /0.0 (977) T1:24.8 /0.0 (977) B@:0 @:0 @0:0 @1:0

I further propose to put the power output in the same order as the temperature output:

ok T:24.8 /0.0 (977) B:25.2 /0.0 (977) T0:24.8 /0.0 (977) T1:24.8 /0.0 (977) @:0 B@:0 @0:0 @1:0

[jbrazio]

I would vote to completely remove the EXTRUDER_WATTS BED_WATTS, we don't have hardware to measure current nor resistance, how can this produce any remotely accurate results ?

And I prefer the ADC to be inline to each extruder/bed output instead of that -> thing, so my vote on would be to change it if possible.

[thinkyhead]

how can this produce any remotely accurate results

It simply displays the proportional "watts" based on your setting for "total watts." So, if your heating power is at 50% and your EXTRUDER_WATTS is set to 10, it will display "5W".

[Blue-Marlin]

Sadly the resistance of the heaters here

// If you want the M105 heater power reported in watts, define the BED_WATTS, and (shared for all extruders) EXTRUDER_WATTS
//#define HOTEND_WATTS (12.0*12.0/6.7) // P=U^2/R
//#define BED_WATTS (12.0*12.0/1.1)    // P=U^2/R

depends on the temperature. So the result is not very exact. Drop it.

I always supposed the raw ADC values output are not supposed to be printed always, but only to make a thermistor table. If this is stated enough - it needs no change.

[jbrazio]

raw ADC values output are not supposed to be printed

Just for debugging proposes of course, that doesn't mean we can't make the output of them more appealing.

[Blue-Marlin]

appealing.

? You drive me crazy. Readable?
What i wanted to say is. The output, if declared as debug output, has not to be machine readable. Humans should be able to read it.

[repetier]

Please do not use
ok T:24.8 /0.0 B:25.2 /0.0 B@:0 @:0 ADC B:977 T0:977

This has 2 times B: with different meanings. That is just screaming for problems (also I'm sure I only parse first B: not expecting more here). T0 would be a problem as we interpret it as temperature and it's not. Don't give one parameter different meanings.

Watts is pretty useless and breaks output which is 0-100% and we don't know 100% here. So just hope nobody selects watt. Watt would only make sense if you add them after space e.g. in brackets. So it is info for users and hosts still get the values 0-127 which they expect anyway. So currently watt is interpreted as 0-127 value and converted in % even if that would be wrong.

What i wanted to say is. The output, if declared as debug output, has not to be machine readable. Humans should be able to read it.

No. M105 is the only way for hosts to get temperatures, so it MUST be machine readable at first place. Additional debug info is ok if it does not change expected response part.

[jbrazio]

Assuming we ditch watts.

ok T:24.8 /0.0 (977) B:25.2 /0.0 (977) T0:24.8 /0.0 (977) T1:24.8 /0.0 (977) @:0 B@:0 @0:0 @1:0

Is this string OK for you ?
Do we require the "space" between this part 24.8 /0.0 ?

[repetier]

Yes the string is ok. Space before / is required as it is a new parameter.

You can keep watts but more like this:

@0:80 (15W)

With a space so it is ignored by parsers as space terminates the value and user reading it gets the idea. But of course it bloats the string and thus more communication.

[jbrazio]

I believe it's clear now, thanks.

[thinkyhead]

Please do not use ok T:24.8 /0.0 B:25.2 /0.0 B@:0 @:0 ADC B:977 T0:977

We can change it going forward. But the output of 1.0.2-1 and previous versions is just as you posted at the top. It's best to use regex and avoid scanf (or its equivalent). The regex to read it looks more or less like this:

(ok +)?T:([\d\.]+) +\/([\d\.]+) +(B:([\d\.]+) +\/([\d\.]+) +)?(T\d:([\d\.]+) +\/([\d\.]+) +)*B@:(\d+W?) +@:(\d+W?) +(@\d:(\d+W?))*( +ADC +B:([\d\.]+(C->(\d+))?) +(T\d:([\d\.]+(C->(\d+))?))?

I'd write it as an extended multi-line regex, but I'm feeling lazy…

When looping through the captured patterns, remember that you already got 'B:', 'T:', and 'T\d:' once. Also look for 'ADC'. Both of these checks will allow you to interpret the second set of 'B:' and 'T:' parameters differently — you probably just want to ignore them.

Of course, you can also do it in steps instead, with a nice small regex to capture all the values and their labels first…

/(([^ :])+:([^ ]+))/g

…and then just pick apart the captured label:value items in-order. This way you don't have to worry that items are in any particular order. But we will continue to keep the ordering well-specified.

[thinkyhead]

Yes the string is ok. Space before / is required as it is a new parameter.

The PR #4452 places ADC values in parentheses so you can ignore them or not.

For the watts, how about moving the 'W' to the label? So instead of this:

@:0W B@:0W @0:0W @1:0W

…this:

@W:0 B@W:0 @0W:0 @1W:0

[jbrazio]

For the watts, how about moving the

Let's take the last breath out of it instead..

[thinkyhead]

@jbrazio @Blue-Marlin Say farewell to James Watt.

[jbrazio]

Watt you gonna do with James ?!

[thinkyhead]

4452 Merged.

[boelle]

I take this one has not been fixed yet?

[jbrazio]

Fixed by #4452.
Please reopen this ticket if the problem still persists.