triffid / FiveD_on_Arduino

Rewrite of reprap mendel firmware
http://forums.reprap.org/read.php?147,33082
GNU General Public License v2.0
30 stars 12 forks source link

Simplify definition of temp sensors and heaters in config.h #19

Closed sw closed 13 years ago

sw commented 13 years ago

Hi,

I found the current method of defining temp sensors and heaters (using HEATER_C and TEMP_C) a bit problematic. It breaks if the order of #includes are modified, for example if in the future some header file will include config.h.

So I changed the way of defining these. In config.h, you can now write:

DEFINE_TEMP_SENSOR(extruder, TT_INTERCOM, 0, 255)

DEFINE_HEATER(extruder, PORTD, PIND6, OCR0A)
DEFINE_HEATER(bed, PORTD, PIND5, OCR0B)

The first parameter is a name (currently ignored), the other three should be obvious (same order as before).

Defining NUM_TEMP_SENSORS and NUM_HEATERS is no longer necessary.

I had to replace some preprocessor checks (#if NUM_TEMP_SENSORS > 1) with normal "if"s, but the compiler will optimize those away if you have no sensors or heaters.

Sorry about the six commits, I still haven't figured out how to make git behave... Is there a way to keep my branch for several pull requests or do I have to delete it each time?

Cheers, Stephan

triffid commented 13 years ago

Re managing git, lots of branches are key: keep your development branch, but make a new branch for each pull request, and cherry pick commits into it. You may be interested in cherry pick's -n option, which allows you to combine multiple commits from your dev branch into one commit on another branch. A rebase then removes redundant commits from your dev.

I will have a look at these commits when I can see them properly on a screen bigger than 10cm :)

triffid commented 13 years ago

I use the heater subsystem for dc extruders as well as fans, and I suspect that ambient temperature sensors will come in at some point.

The reason I chose to put a heater index with each temp sensor is that the temp sensors are checked on a timer, and then need to pass that reading to the heater's PID loop. Having the index with the temp sensor data makes the code for this really simple

I have added M codes for reading arbitrary temp sensors and setting arbitrary heaters but they're not in common usage

triffid commented 13 years ago

Just add HEATER_none = 255 at the end of the enum :)

jgilmore commented 13 years ago

Oh. Yeah, that'd work. You don't even need the "255" just add HEATER_none.

jgilmore commented 13 years ago

Oh, just realized something else. The temp/heater code ALREADY includes config.h multiple times for its side effects. So, this change:

  1. Does not obfusticate things any more than they already are.
  2. Greatly simplifies definition of temp sensors and heaters.
  3. Still allows heaters without temp sensors.
  4. (with one small mod) temp sensors without heaters.

So I now agree with Triffid's initial reaction: Brilliant! Should be included right away. :)