Project: Teacup Firmware

Quote

The only issue is if the interrupt latency is so long (or the delta so short) that the new trigger time has already passed when the interrupt installs the new time- if that happens then the timer must overflow before it triggers.

My hope is, this can't happen with Teacup's current implementation of the stepper timer. I've put in some measures half a year ago to allow delays down to zero. If the commanded delay has passed already, the missed delay is recorded and taken back later. Can't happen too often, or the missed delay whould built up without limit, of course. But it should work 3 or 4 times, sufficient for one step on each motor.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Sometimes funny things happen. After over a year of its implementation I found Teacup's checksumming failed with lower case characters. Anyways, it's fixed now on both, the Gen7 and the master branch.

In case you prefer to feed Teacup with Printrun/Pronterface, use its experimental branch. It has receiced a fix with parsing Teacup's resend requests. These work now, too.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

I'm rewriting dda for now, code is very tight and weird in some places. Also rewrote gcode parser into state machine with saving about 1kbytes of code. Moving very slowly unfortunatly.

x86_64 Wrote:
-------------------------------------------------------
> Also rewrote gcode parser
> into state machine with saving about 1kbytes of
> code.

it already was a state machine

---

-----------------------------------------------
Wooden Mendel
Teacup Firmware

Quote

I'm rewriting dda for now, code is very tight and weird in some places.

Great efforts, x86_64. DDA code is the result of many many iterations; it evolves since about Teacup exists.

Quote

Moving very slowly unfortunatly.

Sometimes it's hard to keep fingers out of a bit code which works, but could be done better. Every code could be done better! But one should decide carefully wether it's worth the effort or wether the time could be invested in more important places.

For my part, I keep changes always as small as possible, even when starting major overhauls. Then push it out to the community to see how it works there. 999 of 1000 RepRappers don't care how confusing a piece of code is, as long as it works, so reliabitily is an very important point. Keeping changes small keeps the chance of pitfalls small, too.

When can I start to cherry-pick the first few of your enhancements?

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Triffid_Hunter Wrote:
-------------------------------------------------------
> it already was a state machine

And now it looks like it too.


Traumflug Wrote:
-------------------------------------------------------
> When can I start to cherry-pick the first few of
> your enhancements?

Well, bad news is that because of gcode processing reconstruction some code is in other files, so cherrypicking would be a little hard. Another bad news is that code so depend on each other that one change immediatly propogate a whole bunch of required changes in other places. Which i think is very bad for any kind of project. However, with new changes it will be less dependent on each other and code will be easier to maintain and stick to your small-changes philosophy.
As for now i keep changes unpushed to be able to rebase and merge commit without any further problems until all code will be set in place. I can push them, but it will be painful to merge later.
Done for now:
- gcode_process splitted into different files, each file for separate hardware piece
- gcode_process not global, but works with passed gcode structure, this allows to execute any code from inside processing itself.
- all queue_wait() and this sort of stuff replaced with internal gcode commands
- time related gcodes moved to separate file too, and will get some internal gcodes too
- gcode_parse works in flex+bison style, with lexer, parser, lexer tokens and parsing states
- dda almost ready for one axis, some troubles with cleaning all up

I still need to rewrite dda_queue properly for one-axis mode, and pinio macros for each axis should be replaced with universal thing.

Whole structure looks like separate hardware features which can be called using gcode, it can call other modules with same gcodes, and for example if i have long running gcode (like stepper move), i don't required to watch over them from all others modules - i just call internal gcode for waiting idle mode and after that i get control. Hopefully this will work.

Quote

Another bad news is that code so depend on each other that one change immediatly propogate a whole bunch of required changes in other places. Which i think is very bad for any kind of project.

I agree the coupling is bad. But it is also bad news about the many touched files. Your changes may need to live in an alternate branch for a while since it sounds like they will conflict with many users' personal, local hacks.

I see your fork on [github.com], but I do not see any of your commits there. I see your gcode_parse changes. I still don't see the dda changes, but I haven't looked too far.

If you push your work up to your fork we may be able to understand more, ask proper questions and make helpful suggestions.

Phil

Edited 1 time(s). Last edit at 08/21/2012 02:21PM by phord.

I've pushed few things for now. Main core is working, parser and core routines works too, i've tested it on real hardware, looks great now. Overall code layout looks more or less what i want to achieve. What it broken is steppers, dda, and a lot of other code is not tested on hardware. I'm still fighting with dda, because of bunch of optimizations some of calculations done in steps, some in um, some use time in miliseconds, some use cpu tick and this whole thing looks messed up. I am trying to keep same calculations as i rewriting it, but feels like i am already got a lot of mistakes.
As for dda and steppers i'll make working version without accelerations soon, and then try to fit accelleration code into it.
If anyone got some time to review whole code for obvious errors, this would helped a lot.

Quote

What it broken is steppers, dda, and a lot of other code

x86_64, I'm tempted to ask you to give your total rewrite a new name (different from "Teacup") or at least to move the discussion of it into a new thread. Even Triffid Hunter himself did, when he rewrote Teacup into Soupcup.

What 99 of 100 RepRappers expect from a firmware is reliability. They're not interested in fancy algorithms or code cleanups as long as they can click to install and click to print.

And this is the RepRap ecosystem, where mistakes and bugs tend to spread like wildfire, yet it takes literally many months until people regain confidentially after the fix. I simply fear somebody actually tries your work and claims "Teacup doesn't work". If that happens, it'll stick for a looong time; a situation which helps nobody. I also fear people find your rework before the real Teacup, as it receives a lot more commits recently. Commits which result in what I cited at the beginning of this post.

Once you think Teacup can gain from your work, I'll happily help to bring it over. In small, clean chunks, so it can be reviewed and quality doesn't suffer. There's nothing stopping us from encapsulating Teacup's parts even better and also nothing stopping us from introducing a fourth axis moving strategy, next to the existing three ones.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Ok, no problems.

With the help of sivu, we could get temperature readouts working for RAMPS electronics. Or all other electronics using analog pins 8 ... 15 for the thermistors. It's on the Gen7 branch for now.

Thanks a lot, sivu!

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

As of today, Teacup allows to control the fan speed with M106 S1 ... M106 S255. Accordingly, M107 (fan off) was removed, as M106 S0 does exactly that.

M109 was also removed, as it's deprecated in the wiki for quite a while now. In case you still used it, replace it with M104, followed by an M116 (in the next line).

As always, all these new things are on the Gen7 branch and will be duplicated to the master branch in a month or so.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Should the example BANG_BANG_OFF in the config files be smaller than the if(value >= 8) test in heater.c/heater_set() routine?

I've finally gotten around to getting a heater working on the $16 teensy 2.0 breadboard, after several missteps. I think the config.h #define BANG_BANG_OFF 45 might have led to part of my confusion, but my burning out the DIO5 pin didn't help with my confusion at all.

Also, the teensy loader seems to clear out the EEPROM, so heater.c/heater_init() reads {P,I,D,Ilim} as {0,0,0,0} with a crc of 0 that appears valid to Teacup. Maybe it would be nice to OR the two PID parameter conditions together and set the parameters to default for the zero case:

                        ...
 			if (((heaters_pid.p_factor == 0) && 
			     (heaters_pid.i_factor == 0) && 
			     (heaters_pid.d_factor == 0) && 
			     (heaters_pid.i_limit  == 0)) ||
			    (crc_block(&heaters_pid.p_factor, 14) != eeprom_read_word((uint16_t *) &EE_factors.crc))) {
				heaters_pid.p_factor = DEFAULT_P;
				heaters_pid.i_factor = DEFAULT_I;
				heaters_pid.d_factor = DEFAULT_D;
				heaters_pid.i_limit = DEFAULT_I_LIMIT;
			}
                        ...

Edited 2 time(s). Last edit at 09/26/2012 04:11PM by DaveX.

Quote

Also, the teensy loader seems to clear out the EEPROM, so heater.c/heater_init() reads {P,I,D,Ilim} as {0,0,0,0} with a crc of 0 that appears valid to Teacup. Maybe it would be nice to OR the two PID parameter conditions together and set the parameters to default for the zero case:

As reading/writing the EEPROM is a pretty essential part for configuration G-code commands like M130 ... M135 (and M200 ... M208 in the future) it might be worth fixing the teensy loader.

If this can't happen, the Teacup-typical way would be to wrap all EEPROM-related stuff in a #define EECONFIG and provide an #else alternative. Applicable patches (the one you gave doesn't show in which file, which line this snippet should be applied) are welcome.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Quote
Traumflug
(the one you gave doesn't show in which file, which line this snippet should be applied)

Scratch that. As I noticed later, you actually did provide a proper patch. Thanks a lot, I applied most of them to the Gen7 branch, the de-facto development branch.


The only open question now is: which behaviour do we want for EEPROM stuff in general?

1) Reset to default after each reset (current behaviour) ?

2) Keep EEPROM settings across resets?

3) Keep EEPROM settings across resets, but save them on command, only. I guess that's Marlin's behaviour, as Marlin provides the G-code M500.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Traumflug Wrote:
-------------------------------------------------------
> (the one you gave doesn't show in which file,
> which line this snippet should be applied)
>
> Scratch that. As I noticed later, you actually did
> provide a proper patch. Thanks a lot, I applied
> most of them to the Gen7 branch, the de-facto
> development branch.
>
>
> The only open question now is: which behaviour do
> we want for EEPROM stuff in general?
>
> 1) Reset to default after each reset (current
> behaviour) ?
>
> 2) Keep EEPROM settings across resets?
>
> 3) Keep EEPROM settings across resets, but save
> them on command, only. I guess that's Marlin's
> behaviour, as Marlin provides the G-code M500.

I like 2 or 3 since the current behavior doesn't trap PID(0,0,0) on my hardware, and default on a cleared EEPROM then does not work. If someone goes through the trouble of setting the EEPROM, it should persist across resets. I like 3 better than two,

The Teensy doesn't clear the eeprom on resets, but does clear it on re-programming/re-flashing. So on a freshly flashed chip the PID values are 0,0,0,0, with 0 stored in the CRC area.

Option 1 isn't the current behavior for me because the fresh-flashed EEPROM default PID of 0,0,0,0 with crc 0 tests as valid, so it does not set the defaults.

The PID(0,0,0,0) is a bad case in that its output is always zero, no matter what the error signal is. BANG-BANG has much better behavior than a PID tuned to 0,0,0. A user is required to dig into the debug options to diagnose this problem since one has to enable debug to access M136 or M111 S1 to even read the PID parameters.

Maybe adding some salt to the CRC test would be a good fix? For example, CRC(0xfeed,0x0,0x0,0x0,0x0) would not be equal to the cleared-EEPROM value of 0x0 and then the default-using branch would then be selected. A user could then still set and save PID(0,0,0,0) if they actually wanted to disable the PID on purpose. But, by default, the PID(0,0,0) case would be trapped and set to the default values.

--- a/crc.c
+++ b/crc.c
@@ -29,7 +29,7 @@
        uses avr-libc's optimised crc16 routine
 */
 uint16_t       crc_block(void *data, uint16_t len) {
-       uint16_t        crc = 0;
+       uint16_t        crc = 0xfeed;
        for (; len; data++, len--) {
                crc = _crc16_update(crc, *((uint8_t *) data));
        }

Edited 2 time(s). Last edit at 09/28/2012 01:58PM by DaveX.

Quote

I like 2 or 3

Looking into the code closer, I see we have M134 "save PID settings to eeprom" already. So I was wrong above when stating we'd have option 1. We apparently have option 3.

Quote

Maybe adding some salt to the CRC test would be a good fix?

Looks good. Have you verified this to work already? Looking at the code I see no issues with it.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Today a user appeared on IRC who's thermistor readouts are apparently flakey enough to never let M116 "wait for temperatures" succeed. I found a (possible) fix in light speed, yet this user disappeared and changed his user name without confirming the success. As I have only a number of Gen7 electronics which has pretty stable temperature readings, I can't really test this fix.

Anyways, I've committed the fix to the Gen7 (aka development) branch. Please test.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

I'm not the user from IRC, but my thermistor circuit is pretty noisy (deltaT for successive readings up to 10C) . I'd been avoiding M116, and the patch worked for me with a TEMP_HYSTERESIS = 10 and TEMP_RESIDENCY_TIME = 20. I didn't realize how bad my thermistor circuit was until now. Using the unfiltered +5 from an ATX power supply rail with a 4.7uf capacitor across the thermistor is not sufficient.

I do think that if noise is what is causing the problem, you should filter it out, either in hardware with better regulation and capacitors, or in software with a filter, before using it in other systems. Otherwise, the noise will play havoc with the systems that depend on it. The downside of filtering the noise is that the system is less responsive, but if you have noise, you don't want it to be responsive to the noise anyway.

Here's a patch that smooths temperature with an Exponentially Weighted Moving Average filter in temp.c:

diff --git a/config.teensy.h b/config.teensy.h
index 7599cbd..b96cd1e 100644
--- a/config.teensy.h
+++ b/config.teensy.h
@@ -342,6 +342,14 @@ New plan:
 */
 #define        TEMP_RESIDENCY_TIME             60

+/**
+        TEMP_EWMA: Exponentially Weighted Moving Average alpha constant for smoothing noisy sensors.  Undefine or set to
+                   1 for unfiltered data.  Instrument Engineer's Handbook, 4th ed, Vol 2 p126 says values of 0.05 to 0.1
+                   are typical.  Smaller is smoother but slower, Larger is quicker but rougher.  Good hardware shouldn't
+                   be noisy.  If you need to use this, set the PID parameter to zero (M132 S0) to make the PID loop
+                   insensitive to noise.  */
+//#define TEMP_EWMA                        0.1
+
 /// which temperature sensors are you using? List every type of sensor you use here once, to enable the appropriate code. Intercom is the gen3-style separate extruder board.
 // #define     TEMP_MAX6675
 #define        TEMP_THERMISTOR
diff --git a/temp.c b/temp.c
index 8247a07..79a1ded 100644
--- a/temp.c
+++ b/temp.c
@@ -289,7 +289,15 @@ void temp_sensor_tick() {
                                default: /* prevent compiler warning */
                                        break;
                        }
-                       temp_sensors_runtime.last_read_temp = temp;
+                        #ifndef TEMP_EWMA
+                          temp_sensors_runtime.last_read_temp = temp;
+                        #else  /* Exponentially smooth the temperature readings */
+                           #define EWMA_SCALE  1024L
+                           #define EWMA_ALPHA  ((long) (TEMP_EWMA * EWMA_SCALE))
+                          temp_sensors_runtime.last_read_temp = (EWMA_ALPHA * temp +
+                                                                   (EWMA_SCALE-EWMA_ALPHA) * temp_sensors_runtime.last_read_temp
+                                                                    ) / EWMA_SCALE;
+                        #endif /* TEMP_EWMA */
                }
                if (labs((int16_t)(temp_sensors_runtime.last_read_temp - temp_sensors_runtime.target_temp)) < (TEMP_HYSTERESIS*4)) {
                        if (temp_sensors_runtime.temp_residency < (TEMP_RESIDENCY_TIME*120))

With my own noisy sensor, the alpha=0.1 smooths the <10C noise to <1C, M116 settles quickly, and the heater PID (using default plus a M132 s0) doesn't look as chaotic. Using the M132 S0 clears the PID's D parameter, which folks should do if they have noise, even if they don't filter the sensor.

Edited 1 time(s). Last edit at 10/02/2012 02:15PM by DaveX.

Traumflug Wrote:
-------------------------------------------------------
> Today a user appeared on IRC who's thermistor
> readouts are apparently flakey enough to never let
> M116 "wait for temperatures" succeed.

Huh!

I guess my thermistor output is pretty flaky, then, because I always assumed M116 was broken. Well, I assumed M109 was broken, because it always just hangs when I encounter it. I never bothered to investigate since it always seems to show up when I'm anxiously doing something else (starting a print).

Should I expect M109 to work, too?

phord:

Without using any patches, you could try increasing TEMP_HYSTERESIS and shortening TEMP_RESIDENCY_TIME to something quick. If all the readings are within TEMP_HYSTERESIS of the setpoint for TEMP_RESIDENCY_TIME, the hang should clear itself.

I guess that's explicitly in the config file:

Quote
config.h
temperature is "achieved" for purposes of M109 and friends when actual temperature is within [hysteresis] of target for [residency] second

Edited 1 time(s). Last edit at 10/02/2012 08:41PM by DaveX.

Quote
phord
I always assumed M116 was broken

The problem was, a single readout of a too low temperature within TEMP_RESIDENCY_TIME (60 seconds by default) did set back counting this time to zero. So, unless you got a stable readout for at least a minute, M116 would never succeed. The newer code allows 10% off readings.

Quote
phord
Should I expect M109 to work, too?

M109 is gone With multiple heaters, M109 is a bit pointless and a M104 followed by a M116 is exactly the same.

Quote
DaveX
Here's a patch that smooths temperature with an Exponentially Weighted Moving Average filter in temp.c:

Thanks a lot, slightly edited patch committed. There's no need for the alternative code, as the optimiser reduces the formula to the trivial case for TEMP_EMWA = 1.0. Also added the index to support multiple sensors.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Traumflug Wrote:
-------------------------------------------------------
> ...
>
> Thanks a lot, slightly edited patch committed.
> There's no need for the alternative code, as the
> optimiser reduces the formula to the trivial case
> for TEMP_EMWA = 1.0. Also added the index to
> support multiple sensors.

Glad you like it. I like EWMAs in work life, and was happy to find an application here.

Additionally, I think [github.com] might be needed for config_teensy.h and arduino_32U4.h to make the AIO*_ADC definitions consistent with the other chips

Quote

Additionally, I think [github.com] might be needed for config_teensy.h and arduino_32U4.h to make the AIO*_ADC definitions consistent with the other chips

That's true. As I can't test this, please provide a patch.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

I've tested [github.com] on my branch/fork/code for a while.

Do you want a patch as is available at that link or a patch like the attached file?

Thanks for the patch, applied to the Gen7 branch. It doesn't matter wether it's a link or a file, a patch made with git format-patch would honor your name into the repositories records.

I've put in an additional patch aligning the new Teensy config template better with config.default.h. This should ease future updates. Unless I made a mistake, it shouldn't change behaviour.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

Looks good, but there might be one issue:

I had a problem with the earlier bang-bang code, and thought that the "#define BANG_BANG_OFF 45" should be less than the heater.c/heater_set threshhold of 8. Since OFF and ON both are larger than 8, wouldn't the defaults make the heater always on?

Excellent observation! Nothing Teensy specific, applies to all kind of electronics. Actually, you made me notice there are code fragments which actually try to do true bang bang, like in pin on - pin off, without PWM. So far I considered Teacup can run heaters with PWM, only.

Looking at the code I fear it's dead code, though. heater_pwm is always set to something in/around line 30 in heater.c, so the lower half of the code in heater_set() is never executed. Also I'm not sure wether code to initialise such a non-PWM heater pin exists.

Any idea on how to re-activate this code? Especially on how to deal with this in config.h without confusing users too much? I'd be very welcome for people using e.g. a heated bed with a solid state relay.

---

Generation 7 Electronics	Teacup Firmware	RepRap DIY

I do not see where heater_pwm is set to 30, but I do see:

	for (i = 0; i < NUM_HEATERS; i++) {
		if (heaters.heater_pwm) {
			*heaters.heater_pwm = 0;

... around line 98 of [github.com]

The heater pins are already set low and output enabled around [github.com] so i don't think you'd need to do more.

I seem to remember that I used the PID code to BANG_BANG a non-PWM pin, but I thought I had to

#define BANG_BANG_OFF 4

to make it work.

I tested "#define BANG_BANG" with mine twice, once with a cold extruder and got wacky temperatures & no heating. And once with a hot extruder and it ran without the PID code (M111 S1 was sparse) but it was hitting the top fork of heater_set.

Maybe one could disable the PWM output by clearing the heater_pwm pointer to zero somehow. The PID loop would execute or not, depending on BANG_BANG. The pid_output and would be filtered to BANG_BANG_[OFF|ON] by the zero in the heater_pwm pointer, and the second fork of heater_set would happen.

Maybe a #define HEATER_BANG_BANG_INDEX 0 for a bit of init code with a dangerous (for bad indexes):

#ifdef HEATER_BANG_BANG_INDEX
heaters[HEATER_BANG_BANG_INDEX].heater_pwm = 0
#endif

Or it would be safer to insert something like this after [github.com] to disable the use of PWM pins across the board

#ifdef BANG_BANG
	for (i = 0; i < NUM_HEATERS; i++) {
		heaters.heater_pwm) = 0;
        }
#endif

... that didn't work. I guess you'd need to do it with some cpp trickery around [github.com]

How about:

diff --git a/heater.c b/heater.c
index 62d5981..b5bb83c 100644
--- a/heater.c
+++ b/heater.c
@@ -28,7 +28,11 @@ typedef struct {
 #undef DEFINE_HEATER
 /// \brief helper macro to fill heater definition struct from config.h
 // #define DEFINE_HEATER(name, port, pin, pwm) { &(port), (pin), &(pwm) },
+#ifndef BANG_BANG
 #define        DEFINE_HEATER(name, pin) { &(pin ## _WPORT), pin ## _PIN, (pin ## _PWM) },
+#else  // BANG_BANG will ignore any possible PWM pins
+#define        DEFINE_HEATER(name, pin) { &(pin ## _WPORT), pin ## _PIN, NULL },
+#endif
 static const heater_definition_t heaters[NUM_HEATERS] =
 {
        #include        "config.h"

Maybe one should do it with a new define, like DISABLE_PWM and then change BANG_BANG into DISABLE_PID or something, since there are two BANG_BANG-like concepts within the code. Someone with a SSR might still want to use the PID to make the control signal less bouncy.

I think I might like this for Teensy's atmega32U4 chip, since the PWMs are pretty odd. E.g, you can't use the PWMs on DIO11 and DIO12 independently, since they use ~OC4D and OC4D. Same for a few of the other PWMs, if you avoid Timer 1 and the 16 bit Timer 3.

Edited 1 time(s). Last edit at 10/07/2012 01:02AM by DaveX.

DaveX Wrote:
-------------------------------------------------------
> Same
> for a few of the other PWMs, if you avoid Timer 1
> and the 16 bit Timer 3.

no need to avoid 16 bit timers for PWM, just set them to 8 bit mode and write to the high word. Should already be some code for the mega to do that

---

-----------------------------------------------
Wooden Mendel
Teacup Firmware