search

neildarlow / Marlin

Marlin 3D Printer Firmware customised for nophead's Mendel90.
6 stars 2 forks source link

Definition of 'analogInputToDigitalPin(p)' seems to be wrong #4

Open pizero opened 10 years ago

pizero commented 10 years ago

That definition 

#define analogInputToDigitalPin(p)  ((p < NUM_ANALOG_INPUTS) ? (p) + 24 : -1)

can be found in

The wrong definition prohibits setting Pin Ext A1 on the extension connector via M42 P30 Snnn. That pin is connected to Port A1, which is Analog Pin 1, Digital Pin 30. The wrong definition maps AI6 (used for the bed temp sensor) to D30 (24+6=30) which in turn rejects M42 P30. It should map AI6 to D25 (31-6=25). Thus the definition should read

#define analogInputToDigitalPin(p)  ((p < NUM_ANALOG_INPUTS) ? 31 - (p) : -1)

since the digital port numbering is reversed on port A.

On a side note: the above files also redefine TIMER5A, TIMER5B, TIMER5C which causes some undesired warnings during compilation. It would be best to just delete these definitions since the 1284P doesn't seem to have these timers.

neildarlow commented 10 years ago

Thanks. I will take a look at this and act appropriately.

neildarlow commented 10 years ago

The analogInputToDigitalPin correction I agree with and will push that.

As far as the TIMER5x redefinition goes, I will need to look into it more and determine if the redefinitions are different and have an impact. Timer-wise, the 1284P only has pyhsical timers 0, 1, and 2 but I need to see how the use of the TIMERx{A,B,C} values translates in Marlin. There is certainly code in Marlin_main.cpp that tests for existence of those defines and modifies registers if they are set.

pizero commented 10 years ago

Thanks for looking into this issue.

The redefinitions are different, as the compiler complains with a warning.

As far as I can see the TIMER definitions are used only in setPwmFrequency(uint8_t pin, int val), which is implemented in Mariln_main.cpp and gets called in temperature.cpp for the FAN_PIN. Whether code is generated depends on the existence of the definition of the related register TCCR5A in iom1284p.h. Thats not defined there, as is to be expected, since the 1284p doesn't have this register, so no code that uses TIMER5x is generated in Marlin for the Melzi.

Redefinitions also occur for FAN_PIN and LED_PIN for the MELZI. The source of this is in pins .h starting with line 1121:

#define LED_PIN -1

#define FAN_PIN -1
#if FAN_PIN == 12 || FAN_PIN ==13
#define FAN_SOFT_PWM
#endif

#ifdef MELZI
#define LED_PIN 27 /* On some broken versions of the Sanguino libraries the pin definitions are wrong, which then needs LED_PIN as pin 28. But you better upgrade your Sanguino libraries! See #368. */
#define FAN_PIN 4 // Works for Panelolu2 too
#endif

As one can see, in case MELZI is defined, LED_PIN and FAN_PIN get redefined. The following would avoid that:

#ifdef MELZI
  #define LED_PIN 27 /* On some broken versions of the Sanguino libraries the pin definitions are wrong, which then needs LED_PIN as pin 28. But you better upgrade your Sanguino libraries! See #368. */
  #define FAN_PIN 4 // Works for Panelolu2 too
#else 
  #define LED_PIN -1
  #define FAN_PIN -1
#endif

#if FAN_PIN == 12 || FAN_PIN ==13
  #define FAN_SOFT_PWM
#endif
neildarlow commented 10 years ago

Yes, there is a lot of nastiness going on here. I have never liked the Arduino idea of defining a pin to be -1 if functionality is not implemented. I have always seen #ifdef as being sufficient.

Regarding warnings (which do not make it past the Arduino IDE anyway) my philosopy is to fix only where something is really broken. I will look at the errant redefinitions and try to determine which is correct and adjust accordingly but I will leave purely cosmetic warnings alone.

pizero commented 10 years ago

Yes it's more of an annoyance, spilling warnings all over the place since these definitions are included many times. It has no effect on the code produced though. I thought I'd mention it since the original distribution from nophead didn't have this problem.

neildarlow commented 10 years ago

OK. The TIMER5A,B,C redefinition errors were due to a missing TIMER4D define in arduino_pins.h. I have added that define rather than deleting TIMER5A,B,C because there is supporting code for TIMER4D in the core directory.

Regarding the redefinition errors for FAN_PIN and LED_PIN, I believe that nophead's code will generate a redefinition warning for FAN_PIN (the code in pins.h does not differ between his and mine) but not for LED_PIN because he commented-out that definition. I re-instated LED_PIN at the request of another user who was prevented from using M42 Sxx to control the LED without this definition being present (he was a little unwilling to change his GCode to M42 P27 Sxx which would have worked without the define).

As a little background to these problems, nophead's Arduino files are pre-version 1.0.1 and his was actually lacking an analogInputToDigitalPin define (which was OK until people started playing with LCD panels). I took the last Sanguino release (which is Arduino version 1.0.1 compliant) and based my files on that. This probably explains any semantic differences in this area between nophead's and my versions.

For the bulk of the Marlin code I took diffs between the source before nophead created his fork and his changes as of the time I created mine. I then reconciled all his changes with a more current Marlin tree (which was quite a task because much of what he had done had since become mainstream but was implemented differently e.g. more functions vs. macros) which puts me where I am today.

My general philosophy is to not do much aesthetic bug fixing on the main code body because I have to periodically merge in the upstream Marlin codebase and I do not want to be dealing with conflicts on a regular basis. I will however continue to add useful little features that are of benefit to Mendel90 users.

lloydw88 commented 4 years ago

Hey guys, so heads up, Im super new to this, so please be gentle. Like many, I have spent hours looking for info on a fix to this timer repetition error. Is there a fix? I have recently tried to add a bl touch to my anet a8. Im as smart to figure out its something to do with the pin and how its hooked up. My printer worked fine, possibly until this. So I would be willing to ask for help from folks smarter than me. Im assuming I can insert a document, so I will upload my configurations.h if someone could give me an idea of where to start, I would be gratious.....

lloydw88 commented 4 years ago

Configuration.zip

lloydw88 commented 4 years ago

[code] /**

/**

ifndef _ENDSTOP_INTERRUPTSH

define _ENDSTOP_INTERRUPTSH

include "macros.h"

// One ISR for all EXT-Interrupts void endstop_ISR(void) { endstops.update(); }

/**

// Install Pin change interrupt for a pin. Can be called multiple times. void pciSetup(const int8_t pin) { SBI(*digitalPinToPCMSK(pin), digitalPinToPCMSKbit(pin)); // enable pin SBI(PCIFR, digitalPinToPCICRbit(pin)); // clear any outstanding interrupt SBI(PCICR, digitalPinToPCICRbit(pin)); // enable interrupt for the group }

// Handlers for pin change interrupts

ifdef PCINT0_vect

ISR(PCINT0_vect) { endstop_ISR(); }

endif

ifdef PCINT1_vect

ISR(PCINT1_vect) { endstop_ISR(); }

endif

ifdef PCINT2_vect

ISR(PCINT2_vect) { endstop_ISR(); }

endif

ifdef PCINT3_vect

ISR(PCINT3_vect) { endstop_ISR(); }

endif

void setup_endstop_interrupts( void ) {

if HAS_X_MAX

#if digitalPinToInterrupt(X_MAX_PIN) != NOT_AN_INTERRUPT // if pin has an external interrupt
  attachInterrupt(digitalPinToInterrupt(X_MAX_PIN), endstop_ISR, CHANGE); // assign it
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(X_MAX_PIN) != NULL, "X_MAX_PIN is not interrupt-capable"); // if pin has no pin change interrupt - error
  pciSetup(X_MAX_PIN);                                                            // assign it
#endif

endif

if HAS_X_MIN

#if digitalPinToInterrupt(X_MIN_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(X_MIN_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(X_MIN_PIN) != NULL, "X_MIN_PIN is not interrupt-capable");
  pciSetup(X_MIN_PIN);
#endif

endif

if HAS_Y_MAX

#if digitalPinToInterrupt(Y_MAX_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(Y_MAX_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Y_MAX_PIN) != NULL, "Y_MAX_PIN is not interrupt-capable");
  pciSetup(Y_MAX_PIN);
#endif

endif

if HAS_Y_MIN

#if digitalPinToInterrupt(Y_MIN_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(Y_MIN_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Y_MIN_PIN) != NULL, "Y_MIN_PIN is not interrupt-capable");
  pciSetup(Y_MIN_PIN);
#endif

endif

if HAS_Z_MAX

#if digitalPinToInterrupt(Z_MAX_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(Z_MAX_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Z_MAX_PIN) != NULL, "Z_MAX_PIN is not interrupt-capable");
  pciSetup(Z_MAX_PIN);
#endif

endif

if HAS_Z_MIN

#if digitalPinToInterrupt(Z_MIN_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(Z_MIN_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Z_MIN_PIN) != NULL, "Z_MIN_PIN is not interrupt-capable");
  pciSetup(Z_MIN_PIN);
#endif

endif

if HAS_X2_MAX

#if (digitalPinToInterrupt(X2_MAX_PIN) != NOT_AN_INTERRUPT)
  attachInterrupt(digitalPinToInterrupt(X2_MAX_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(X2_MAX_PIN) != NULL, "X2_MAX_PIN is not interrupt-capable");
  pciSetup(X2_MAX_PIN);
#endif

endif

if HAS_X2_MIN

#if (digitalPinToInterrupt(X2_MIN_PIN) != NOT_AN_INTERRUPT)
  attachInterrupt(digitalPinToInterrupt(X2_MIN_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(X2_MIN_PIN) != NULL, "X2_MIN_PIN is not interrupt-capable");
  pciSetup(X2_MIN_PIN);
#endif

endif

if HAS_Y2_MAX

#if (digitalPinToInterrupt(Y2_MAX_PIN) != NOT_AN_INTERRUPT)
  attachInterrupt(digitalPinToInterrupt(Y2_MAX_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Y2_MAX_PIN) != NULL, "Y2_MAX_PIN is not interrupt-capable");
  pciSetup(Y2_MAX_PIN);
#endif

endif

if HAS_Y2_MIN

#if (digitalPinToInterrupt(Y2_MIN_PIN) != NOT_AN_INTERRUPT)
  attachInterrupt(digitalPinToInterrupt(Y2_MIN_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Y2_MIN_PIN) != NULL, "Y2_MIN_PIN is not interrupt-capable");
  pciSetup(Y2_MIN_PIN);
#endif

endif

if HAS_Z2_MAX

#if digitalPinToInterrupt(Z2_MAX_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(Z2_MAX_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Z2_MAX_PIN) != NULL, "Z2_MAX_PIN is not interrupt-capable");
  pciSetup(Z2_MAX_PIN);
#endif

endif

if HAS_Z2_MIN

#if digitalPinToInterrupt(Z2_MIN_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(Z2_MIN_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Z2_MIN_PIN) != NULL, "Z2_MIN_PIN is not interrupt-capable");
  pciSetup(Z2_MIN_PIN);
#endif

endif

if HAS_Z_MIN_PROBE_PIN

#if digitalPinToInterrupt(Z_MIN_PROBE_PIN) != NOT_AN_INTERRUPT
  attachInterrupt(digitalPinToInterrupt(Z_MIN_PROBE_PIN), endstop_ISR, CHANGE);
#else
  // Not all used endstop/probe -pins can raise interrupts. Please deactivate ENDSTOP_INTERRUPTS or change the pin configuration!
  static_assert(digitalPinToPCICR(Z_MIN_PROBE_PIN) != NULL, "Z_MIN_PROBE_PIN is not interrupt-capable");
  pciSetup(Z_MIN_PROBE_PIN);
#endif

endif

// If we arrive here without raising an assertion, each pin has either an EXT-interrupt or a PCI. }

endif // _ENDSTOP_INTERRUPTSH

[/code]

neildarlow commented 4 years ago

I'm afraid I can't help with your situation on several counts:

1) You are working with Marlin 2 and this codebase is Marlin 1 2) Your controller uses the ATmega 2560 which differs from the ATmega 1284p used here 3) A software project like this specific Marlin port cannot support other hardware

i would suggest you approach an ANet forum where, I'm sure, people will have done what you are attempting.