Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

analog pin paramter for analogRead not zero based #1766

Closed
jnsbyr opened this issue Mar 13, 2016 · 8 comments
Closed

analog pin paramter for analogRead not zero based #1766

jnsbyr opened this issue Mar 13, 2016 · 8 comments

Comments

@jnsbyr
Copy link

jnsbyr commented Mar 13, 2016

Basic Infos

Hardware

Hardware: ESP8266 - all boards
Core Version: 2.1.0

Description

The analog pin parameter for analogRead() is currently not zero based but absolute (A0 = 17) in contrast to the standard Arduino library implementation (see https://www.arduino.cc/en/Reference/AnalogRead). This makes ports of existing projects (e.g. https://github.com/firmata/arduino) harder.

Possible Solution

  • change in all variants/.../pins_arduino.h:
...
static const uint8_t A0 = 0;
...
  • change in core_esp8266_wiring_analog.c:
...
extern int __analogRead(uint8_t pin) {
  if(pin == 0){
    return system_adc_read();
  }
  return digitalRead(pin) * 1023;
}
...
@igrr
Copy link
Member

igrr commented Mar 13, 2016

It's not zero based for at least two variants of the AVR core:
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/variants/standard/pins_arduino.h#L47-L54
https://github.com/arduino/Arduino/blob/master/hardware/arduino/avr/variants/leonardo/pins_arduino.h#L114-L125

Edit:
the actual implementation of analogRead allows passing in both A0 (i.e. number of pin) and 0 (number of the analog channel). I think we can adjust analogRead implementation to handle both values, like AVR analogRead does. I don't think that changing A0 back to 0 is a good idea though, as that would imply that analog input is on pin 0, which isn't the case.

@jnsbyr
Copy link
Author

jnsbyr commented Mar 13, 2016

Thx for considering the suggestion. Keeping A0 at 17 will not be a problem if analogRead works with pin=0.

@jnsbyr
Copy link
Author

jnsbyr commented Mar 15, 2016

An analysis has shown that the wifio board has some extras that need to be considered, especially A1 to A7 in addition to A0. Best I can think of would be to do it like we agreed for A0 and support access both absolute and zero based. There are no overlaps between absolute pin numbers and zero based pin numbers even with wifio so this should be possible. This issue blocks the integration of the ESP8266 into firmata/arduino (see firmata/arduino#278 (comment)).

@igrr
Copy link
Member

igrr commented Mar 16, 2016

@jnsbyr this is now fixed in git version.

@jnsbyr
Copy link
Author

jnsbyr commented Mar 16, 2016

Thanks, will let you know how it works.

@jnsbyr
Copy link
Author

jnsbyr commented Mar 18, 2016

With the new version analogRead() now works for A0 as expected.

There are similar problems with the macros digitalPinToInterrupt(p) and digitalPinHasPWM(p) from pins_arduino.h. E.g. the second macro returns true for pins 1 to 17 with an Adafruit board. But it should be 0 to 16 or even better 0 to 5 and 12 to 16, because you should not do PWM on your flash IOs. The reason is that the current macro uses NOT_A_PIN but that resolves to -1 and that is true for 17 and it returns the pin number and that is false for 0.

My suggestions:

  • minimum change
#define digitalPinToInterrupt(p)    (((p) < EXTERNAL_NUM_INTERRUPTS)?1:0)
#define digitalPinHasPWM(p)         (((p) < NUM_DIGITAL_PINS)?1:0)
  • saveguard approach
#define digitalPinToInterrupt(p)    (((p) < EXTERNAL_NUM_INTERRUPTS && ((p) < 6 || (p) > 11))?1:0)
#define digitalPinHasPWM(p)         (((p) < NUM_DIGITAL_PINS && ((p) < 6 || (p) > 11))?1:0)

EDIT:
Problem with macros raised as issue #1831

@jnsbyr
Copy link
Author

jnsbyr commented Apr 1, 2016

@igrr
From my point of view this issue is resolved and can be closed.

@igrr
Copy link
Member

igrr commented Apr 1, 2016

@jnsbyr Thanks! It will get closed (automatically) once we deploy the new release (2.2.0).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants