IDE 1.5.3 bugs / issues

darrylp · September 3, 2013, 10:28am

for the avr core wiring_analog.c

how is the following bit of code any good as compared to 1.5.2 ?

analogPinToChannel is not defined for standard and mega, in fact its only defined in variants/lenardo and the robot_control [& motor] the double check and action for the ATmega32U4 is so good ! not !!!

int analogRead(uint8_t pin)
{
    uint8_t low, high;

#if defined(analogPinToChannel)
#if defined(__AVR_ATmega32U4__)
    if (pin >= 18) pin -= 18; // allow for channel or pin numbers
#endif
    pin = analogPinToChannel(pin);
#elif defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__)
    if (pin >= 54) pin -= 54; // allow for channel or pin numbers
#elif defined(__AVR_ATmega32U4__)
    if (pin >= 18) pin -= 18; // allow for channel or pin numbers
#elif defined(__AVR_ATmega1284__) || defined(__AVR_ATmega1284P__) || defined(__AVR_ATmega644__) || defined(__AVR_ATmega644A__) || defined(__AVR_ATmega644P__) || defined(__AVR_ATmega644PA__)
    if (pin >= 24) pin -= 24; // allow for channel or pin numbers
#else
    if (pin >= 14) pin -= 14; // allow for channel or pin numbers
#endif

darrylp · September 3, 2013, 12:02pm

and another similar bug in HardwareSerial.cpp for the avr

  #if defined(UDR0)
    if (bit_is_clear(UCSR0A, UPE0)) {
      unsigned char c = UDR0;
      store_char(c, &Serial);
    } else {
      unsigned char c = UDR0;
    };
  #elif defined(UDR)
    if (bit_is_clear(UCSRA, PE)) {
      unsigned char c = UDR;
      store_char(c, &rx_buffer);    <----------  rx_buffer doesn't exist, think its meant to be &Serial here !
    } else {
      unsigned char c = UDR;
    };
  #else
    #error UDR not defined
  #endif

drjiohnsmith · September 3, 2013, 4:36pm

are u surprised ?

the arduino team will say its beta,

but..

darrylp · September 4, 2013, 9:00am

drjiohnsmith: are u surprised ?

the arduino team will say its beta,

but..

no of course not surprised, just thought someone who is on the developer list might actually watch in here. 1.5.4 is going to need the two things fixing as otherwise its breaking existing code base. first with the commented out pin number adapting, affects all boards except lenardo and robot ones, and the other is for processors that have UDR defined instead of UDR0 ( no idear what they are Attiny maybe ? )

cmaglie · September 9, 2013, 10:07am

@darryl

thanks for finding the HardwareSerial error.

About the analogPinToChannel, I don't understand if you find another bug? You can find some discussion about the patch here:

https://github.com/arduino/Arduino/issues/1366 https://github.com/arduino/Arduino/pull/1368

darrylp · September 9, 2013, 3:42pm

cmaglie: @darryl

thanks for finding the HardwareSerial error.

About the analogPinToChannel, I don't understand if you find another bug? You can find some discussion about the patch here:

https://github.com/arduino/Arduino/issues/1366 https://github.com/arduino/Arduino/pull/1368

as per the discussion in the two links, the non defining of "analogPinToChannel" blocks out the code in the first message of this thread, for example any code accessing the ADC pins of a Mega 2560, will no longer work.

the code is alos badly formed, follow the trace for example of a Due ( which has AVR_ATmega32U4 defined ? )

you will see its taking off 18 twice from the pin number....

here is the code again, but indented to show the nesting clearer

#if defined(analogPinToChannel)
    #if defined(__AVR_ATmega32U4__)
        if (pin >= 18) pin -= 18; // allow for channel or pin numbers
    #endif
    pin = analogPinToChannel(pin);
#elif defined(__AVR_ATmega1280__) || defined(__AVR_ATmega2560__)
    if (pin >= 54) pin -= 54; // allow for channel or pin numbers
#elif defined(__AVR_ATmega32U4__)
    if (pin >= 18) pin -= 18; // allow for channel or pin numbers
#elif defined(__AVR_ATmega1284__) || defined(__AVR_ATmega1284P__) || defined(__AVR_ATmega644__) || defined(__AVR_ATmega644A__) || defined(__AVR_ATmega644P__) || defined(__AVR_ATmega644PA__)
    if (pin >= 24) pin -= 24; // allow for channel or pin numbers
#else
    if (pin >= 14) pin -= 14; // allow for channel or pin numbers
#endif

I dont use a arduino board with a processor that defines the following.... AVR_ATmega32U4 but I expect it wont be what it is expected if you dont use the low numbers to accessthe ADC pins.

cmaglie · September 9, 2013, 4:22pm

darryl: as per the discussion in the two links, the non defining of "analogPinToChannel" blocks out the code in the first message of this thread, for example any code accessing the ADC pins of a Mega 2560, will no longer work.

@darryl Sorry, but I still can't understand where the issue is, may you provide an example that fails?

I tried to compile for Mega2560 and the compiler preprocessor selected:

 if (pin >= 54) pin -= 54;

as expected.

darryl: the code is alos badly formed, follow the trace for example of a Due ( which has AVR_ATmega32U4 defined ? )

The Due doesn't have AVR_ATmega32U4 defined, and use a totally different wiring_analog.c, you should look here for the Due:

https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/cores/arduino/wiring_analog.c

darrylp · September 9, 2013, 7:36pm

cmaglie:

darryl: as per the discussion in the two links, the non defining of "analogPinToChannel" blocks out the code in the first message of this thread, for example any code accessing the ADC pins of a Mega 2560, will no longer work.

@darryl Sorry, but I still can't understand where the issue is, may you provide an example that fails?

I tried to compile for Mega2560 and the compiler preprocessor selected:
 if (pin >= 54) pin -= 54;
as expected.

darryl: the code is alos badly formed, follow the trace for example of a Due ( which has AVR_ATmega32U4 defined ? )

The Due doesn't have AVR_ATmega32U4 defined, and use a totally different wiring_analog.c, you should look here for the Due:

https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/sam/cores/arduino/wiring_analog.c

Oh dear, I can now see after sitting and studying the code, not using my default editor, that a bug in the editor is highlighting and code matching incorrectly. I feel so silly ! It has however shown a problem that the nesting of c/c++ defines and conditionals is now broken ! just not the arduino code, but my editor !

mentioning the due, I of course meant the lenardo ( as per the first post ) so please forgive me trying to convince you of some bad code.... at least the second post had a real bug !

sorry again !

Lauszus · September 9, 2013, 8:40pm

@darryl The reason why the pin number gets subtracted by 18 when you are using a Leonardo is because I didn't want to break backwards compatibility with third party versions of the Leonardo. Take a look at the original code before my pull request: https://github.com/arduino/Arduino/blob/aba27c43aa1d4ea827c8a07774e7ea398cd7b259/hardware/arduino/avr/cores/arduino/wiring_analog.c#L46-L57, as you can see it subtracts 18 before analogPinToChannel is called.

I know that I could just have removed it and then fixed it in the variants file: https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/avr/variants/leonardo/pins_arduino.h#L133. That would have worked just fine with the official Leonardo, but would have broken analog reading on third party boards using a ATmega32U4 with it's own variants file.

If you still thinks it's a bug, then fell free to open up another pull request that removes this, but I doubt it will get merged because of it's incompatibility with existing third party boards.

darrylp · September 9, 2013, 10:20pm

Lauszus: @darryl The reason why the pin number gets subtracted by 18 when you are using a Leonardo is because I didn't want to break backwards compatibility with third party versions of the Leonardo. Take a look at the original code before my pull request: https://github.com/arduino/Arduino/blob/aba27c43aa1d4ea827c8a07774e7ea398cd7b259/hardware/arduino/avr/cores/arduino/wiring_analog.c#L46-L57, as you can see it subtracts 18 before analogPinToChannel is called.

I know that I could just have removed it and then fixed it in the variants file: https://github.com/arduino/Arduino/blob/ide-1.5.x/hardware/arduino/avr/variants/leonardo/pins_arduino.h#L133. That would have worked just fine with the official Leonardo, but would have broken analog reading on third party boards using a ATmega32U4 with it's own variants file.

If you still thinks it's a bug, then fell free to open up another pull request that removes this, but I doubt it will get merged because of it's incompatibility with existing third party boards.

no, the code is good. I was reading what my IDE editor was showing me as code to be used for my compile options, rather than what the gcc pre-processor would actually use. it has a bug, that i will look into tomorrow when back at work :-)

cmaglie · September 10, 2013, 7:40am

darryl: so please forgive me trying to convince you of some bad code.... at least the second post had a real bug !

No worries! ;-)