[teacup] GCODE_COMMAND.S: insufficient range

int16_t S member is being assigned with return value from "int32_t decfloat_to_int(decfloat *df, uint16_t multiplicand)" and used to pass that value into pid_set_*(heater_t, int32_t) functions. This becomes problem especially with M132 command which multiplies passed S value (~100) with PID_SCALE (1024).

An M132 S100 sets the PID D value to 100 PWM counts per quarter degree per 2 seconds dT/dt, or IOW, to +/- full scale (+/-255) for a 0.31C/second temperature change rate. Are you sure that's a desirable behavior?

I've been working with the PID units in Teacup a bit, (https://github.com/Traumflug/Teacup_Firmware/issues/74) and running into the int16_t/1024 limits, but I'm not sure of the right balance.

Thanks, I had just used values from here. So from that github discussion it seems that those values are supposed to be scaled down by factors 4, 16, 8 to work with teacup. I know that I'm supposed to use my own values, but those, after scaled down, work much better than defaults for me.

I like the lack of teacup dependency on arduino, but sadly, that discussion convinced me, that its eventually time to move on.

Quote
miso
Thanks, I had just used values from here. So from that github discussion it seems that those values are supposed to be scaled down by factors 4, 16, 8 to work with teacup. I know that I'm supposed to use my own values, but those, after scaled down, work much better than defaults for me.

Yes, those values look like marlin-formatted values, which are in units of {PWM/C. PWM/Cs, PWM/(C/s)} Teacup, with M130-133 sets the PID values are in units that conform to the internal/raw measurement units, but are relatively hard to interpret. I've been working on making them be in more standard units, but the M13? S???? filter through the int16_t was a bit awkward. I was avoiding changing the type of the S parsing in case it screwed anything else up, but now that you've brought it up, it seems like it doesn't break anything and the downside would be only 8 bytes more flash. Thanks. I was having a hard time fitting high gain processes with slow integrals into the current scheme.

Quote
miso
I like the lack of teacup dependency on arduino, but sadly, that discussion convinced me, that its eventually time to move on.

Sorry to hear that. If the discussion there about jerkyness was the clincher, Teacup now does lookahead.

I like Teacup because it was much easier to see what was going on and to extend to an ATmega32u. PID-wise, it does allow for multiple tunings for different heaters, like M130 P2 S32, etc... For a workaround on Marlin, you might need to reset the PID parameters with each toolchange with M130.

Quote
DaveX
it seems like it doesn't break anything and the downside would be only 8 bytes more flash.

Plus 2 bytes of RAM, but it doesn't seem like the memory was an issue when the parser was designed, because that "union of all parameters" struct approach is inherently memory hungry:

uint8_t                                         G;                              ///< G command number
uint8_t                                         M;                              ///< M command number

Since "G" and "M" codes are AFAIK mutually exclusive, one doesn't need to allocate space to store both of those at same time. Single member like "uint8_t cmd_code" should suffice. Same is probably true for "S" parameter of M13[0-3] commands, (or maybe S parameter of any command) it could be probably stored in some field of TARGET struct.

Quote
DaveX
Sorry to hear that. If the discussion there about jerkyness was the clincher, Teacup now does lookahead.

Quote
Traumflug
But ... does this really help? My guess is, such values are voodoo for 99% of the users anyways, no matter which units they are.

Apparently maintainer has different opinion (doubts) on things that are important (sure thing) to me, so its likely that in the future the project will part even further from my expectations.

When I was "porting" my configuration from teacup to marlin, I have discovered, that feed rate limits in teacups configuration.h, that are claimed to be in mm/minute are actually in mm/6 minutes.
I need to set:

#define MAXIMUM_FEEDRATE_Z                      300

(6*50) to make G0 Z50 command finish in one minute. Can anybody confirm?

Sorry for late response.

Quote
miso
it doesn't seem like the memory was an issue when the parser was designed, because that "union of all parameters" struct approach is inherently memory hungry:

uint8_t                                         G;                              ///< G command number
uint8_t                                         M;                              ///< M command number

Since "G" and "M" codes are AFAIK mutually exclusive, one doesn't need to allocate space to store both of those at same time. Single member like "uint8_t cmd_code" should suffice.

You spotted a possible improvement. Nice! Please form a patch, test it and it'll be applied.

That said, I'm aware of even more room for improvement. The looping branch cuts down flash size by more than 1000 bytes. Such stuff has to be verified, tested and checked wether it degrades performance (maximum step pulse speed). No paid developers available, so it needs volunteers.

Quote
miso
When I was "porting" my configuration from teacup to marlin, I have discovered, that feed rate limits in teacups configuration.h, that are claimed to be in mm/minute are actually in mm/6 minutes.
I need to set:

#define MAXIMUM_FEEDRATE_Z                      300

(6*50) to make G0 Z50 command finish in one minute. Can anybody confirm?

Sort of. On a configuration with all three axes having the same step/m value I get the expected 10 seconds. On configurations where steps/m is bigger for Z than for X, Z is slower. On a typical Mendel configuration I get these 60 seconds you observe. Distances are all apparently accurate, though.

That said, thanks for measuring, thanks for telling. It sometimes takes ages until users reports something in a fashion which allows investigation.

Trying to track the cause down left me puzzled for quite a while. All variables starting a move were apparently correct. It's not related to movement speed limitation, but observable at any move (like G1 Z50 F300). Until ... well the reason in a development shortcut found in dda_maths.h, line 75f:

// Initialization constant for the ramping algorithm.
#define C0 (((uint32_t)((double)F_CPU / sqrt((double)(STEPS_PER_M_X * ACCELERATION / 2000.)))) << 8)

This is used for calculation of accelerated speeds and as all movements are accelerated, speeds are calculated for steps/m values of the X axis, not the axis actually moving. A fix isn't trivial, because as you see, it involves floating point maths which isn't allowed at runtime. Relevant usage of C0 is in dda.c, line 850ff, in case you want to work on a fix yourself.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY