pin functions based on board depended defaults #278

jnsbyr · 2016-03-13T11:22:38Z

to support different ESP8266 boad layouts, use board depended constants
from "...\esp8266\hardware\esp8266\2.1.0\variants...\pins_arduino.h"

DOUT, DIN, I2C and SERIAL0 have been tested and are working with a HUZZA board

to support different ESP8266 boad layouts, use board depended constants from "...\esp8266\hardware\esp8266\2.1.0\variants\...\pins_arduino.h"

requires Arduino core for ESP8266 V2.2

soundanalogous · 2016-03-13T18:28:06Z

Boards.h

-#define IS_PIN_SPI(p)           (false)
+#define TOTAL_ANALOG_PINS       1
+#define TOTAL_PINS              18 // 11 digital + 1 analog + 6 inaccessible
+#define VERSION_BLINK_PIN       LED_BUILTIN


Remove VERSION_BLINK_PIN. The version blink sequence takes too long and excluding this define will exclude the version blink sequence - saving up to 2.5 seconds or more of startup time (since each Firmata version increment adds additional time to the sequence).

will be removed with next commit

jnsbyr · 2016-03-13T20:21:27Z

Note that with current release V2.1 of esp8266/Arduino you need

#if !defined(ARDUINO_ARCH_ESP8266)
          Firmata.sendAnalog(analogPin, analogRead(analogPin));
#else
          Firmata.sendAnalog(analogPin, analogRead(A0));
#endif

to make the analog input work. I opened an issue (esp8266/Arduino#1766) to make the define unnecessary.

rwaldron · 2016-03-13T20:25:02Z

Boards.h

+#define PIN_SERIAL_TX           1
+#define PIN_SERIAL1_TX          2
+#define IS_PIN_DIGITAL(p)       (((p) >= 0 && (p) <= 5) || ((p) >= 12 && (p) <= 16))
+#define IS_PIN_ANALOG(p)        ((p) == A0)


What about the wifio?

static const uint8_t A0 = 14; static const uint8_t A1 = 15; static const uint8_t A2 = 16; static const uint8_t A3 = 17; static const uint8_t A4 = 18; static const uint8_t A5 = 19; static const uint8_t A6 = 20; static const uint8_t A7 = 21;

?

Please tell me where these constants are needed for. A0 = 17 is already in use in the esp8266/Arduino project for the one and only analog input pin.

OK, found it myself. For this board probably a separate definition is needed in Board.h and that may not be enough to make it work.

soundanalogous · 2016-03-13T21:04:55Z

As much as I hate to add yet another StandardFirmata variant (mostly because it sucks to maintain them all), we may need to add StandardFirmataESP and include espConfig.h (much in the same way I've included a config for Ethernet, WiFi and BLE). That way the user can uncomment a define that will configure the sketch properly for their particular board.

soundanalogous · 2016-03-13T21:06:30Z

Or it could all be handled in wifiConfig.h (in StandardFirmataWiFi) - provide different configurations and the user selects one by uncommenting an appropriate define.

soundanalogous · 2016-03-13T21:10:11Z

I see now the wifiConfig.h route is what Jacob had started so lets continue down that route. Also need to figure out a better way to manage this as it's starting to get out of hand.

In wifiConfig you can manage configurations per specific ESP board.

soundanalogous · 2016-03-13T21:24:42Z

One thing that would make this much easier is if each variant in esp8266/Arduino had a unique define - then we could sort these out easily in Boards.h.

jnsbyr · 2016-03-13T21:29:18Z

For my tests I didn't need to go so far as the changes you refer to. I based my tests on StandardFirmataEthernetPlus, changed Ethernet into WiFi, added a few lines to setup WiFi and added a few defines to exclude the Arduino board special pin configurations.

Having a unique identifier for the different boards would help. Maybe the project is open to such an enhancement.

soundanalogous · 2016-03-13T21:34:14Z

One way to have a different unique identifier is to have a unique sketch name for each board and inspect that name view the firmware query (which returns the filename)

soundanalogous · 2016-03-13T21:45:02Z

I looked over the pins_arduino.h files for all of the variants in esp8266/Arduino. The following should cover everything except the wifio board (which would still work but the offset pins 20+ would not work). The serial pin mapping may not be correct for all boards. Also some digital pins may need to be ignored (and can be in wifiConfig.h if necessary):

#elif defined(ESP8266)
#define TOTAL_ANALOG_PINS       NUM_ANALOG_INPUTS
#define TOTAL_PINS              A0 + NUM_ANALOG_INPUTS
#define PIN_SERIAL_RX           3
#define PIN_SERIAL_TX           1
#define IS_PIN_DIGITAL(p)       ((p) >= 0 && (p) < A0)
#define IS_PIN_ANALOG(p)        ((p) >= A0 && (p) < A0 + NUM_ANALOG_PINS)
#define IS_PIN_PWM(p)           digitalPinHasPWM(p)
#define IS_PIN_SERVO(p)         (IS_PIN_DIGITAL(p) && (p) < MAX_SERVOS)
#define IS_PIN_I2C(p)           ((p) == SDA || (p) == SCL)
#define IS_PIN_SPI(p)           ((p) == SS || (p) == MOSI || (p) == MISO || (p) == SCK)
#define IS_PIN_INTERRUPT(p)     digitalPinToInterrupt(p)
#define IS_PIN_SERIAL(p)        ((p) == 1 || (p) == 3)
#define PIN_TO_DIGITAL(p)       (p)
#define PIN_TO_ANALOG(p)        ((p) - A0)
#define PIN_TO_PWM(p)           PIN_TO_DIGITAL(p)
#define PIN_TO_SERVO(p)         (p)

jnsbyr · 2016-03-13T22:18:35Z

Looks good to me, but I would explicitly exclude the pins 6 to 11. They are used for the flash IO on most of the boards and accessing them by accident will probably crash the application. Using the higher number of the total pins constant from pins_arduino.h for an ifdef one could try to make a special config for wifio.

soundanalogous · 2016-03-13T22:53:16Z

Untested, but this may cover all boards:

#elif defined(ESP8266)
#define TOTAL_ANALOG_PINS       NUM_ANALOG_INPUTS
#define TOTAL_PINS              (NUM_DIGITAL_PINS > A0 + NUM_ANALOG_INPUTS ? NUM_DIGITAL_PINS : A0 + NUM_ANALOG_INPUTS)
#define PIN_SERIAL_RX           3
#define PIN_SERIAL_TX           1
#define IS_PIN_DIGITAL(p)       (((p) >= 0 && (p) <= 5) || ((p) >= 12 && (p) < (NUM_DIGITAL_PINS > A0 + NUM_ANALOG_INPUTS ? NUM_DIGITAL_PINS : A0)))
#define IS_PIN_ANALOG(p)        ((p) >= A0 && (p) < A0 + NUM_ANALOG_PINS)
#define IS_PIN_PWM(p)           digitalPinHasPWM(p)
#define IS_PIN_SERVO(p)         (IS_PIN_DIGITAL(p) && (p) < MAX_SERVOS)
#define IS_PIN_I2C(p)           ((p) == SDA || (p) == SCL)
#define IS_PIN_SPI(p)           ((p) == SS || (p) == MOSI || (p) == MISO || (p) == SCK)
#define IS_PIN_INTERRUPT(p)     digitalPinToInterrupt(p)
#define IS_PIN_SERIAL(p)        ((p) == 1 || (p) == 3)
#define PIN_TO_DIGITAL(p)       (p)
#define PIN_TO_ANALOG(p)        ((p) - A0)
#define PIN_TO_PWM(p)           PIN_TO_DIGITAL(p)
#define PIN_TO_SERVO(p)         (p)

jnsbyr · 2016-03-14T20:24:15Z

Don't know why but I had trouble with the line #define IS_PIN_PWM(p) digitalPinHasPWM(p). The capability query returned all pins and not only the digital pins so I changed it into #define IS_PIN_PWM(p) IS_PIN_DIGITAL(p). Maybe the xtensa compiler works a little bit different when resolving macros.

jnsbyr · 2016-03-14T20:40:50Z

Because of esp8266/Arduino#1766 #define PIN_TO_ANALOG(p) ((p) - A0) will not help with wifio A1 to A7. Either the esp8266/Arduino project also remaps these pins for analogRead() or one adds a special reverse mapping to Firmata for calling analogRead().

soundanalogous · 2016-03-14T21:09:47Z

I don't see how that would be an issue since wifio A0 is mapped to pin 14 as it should be for that board: https://github.com/esp8266/Arduino/blob/master/variants/wifio/pins_arduino.h#L41.

PIN_TO_ANALOG should work correctly since 14 - 14 = 0, 15 - 14 = 1, etc.

jnsbyr · 2016-03-14T21:32:11Z

I did not say that it's not possible but that the esp project V2.1 assumes that analogRead is called with the absolute pin number. Even A0 is currently only working by calling analogRead(17) but PIN_TO_ANALOG would call analogRead(0) instead.

soundanalogous · 2016-03-14T22:36:45Z

The broken pin mapping in esp8266 is definitely a blocker. Hopefully they resolve that soon.

igrr · 2016-03-15T19:17:20Z

Guys, you may ignore wifio variant — it's a board which was never released. We keep the code in repository because it shows how to implement "virtual" IO with an AVR slave. Noone is actually using it.

I'll fix analogRead(0) asap.

rwaldron · 2016-03-15T21:40:01Z

@igrr thanks for the details :)

…ts from pins_arduino.h

jnsbyr · 2016-03-18T19:54:01Z

#define IS_PIN_PWM(p) digitalPinHasPWM(p) results in all pins including the analog pin claiming to have PWM capability. The same probably applies to #define IS_PIN_INTERRUPT(p) digitalPinToInterrupt(p) but that macro is not used in Firmata afaik
Firmata 2.5 does not compile without VERSION_BLINK_PIN defined
analog pin A0 works with current git version of "cores/esp8266/core_esp8266_wiring_analog.c" on Huzza board

soundanalogous · 2016-03-18T20:17:11Z

#define IS_PIN_PWM(p) digitalPinHasPWM(p) results in all pins including the analog pin claiming to have PWM capability. The same probably applies to #define IS_PIN_INTERRUPT(p) digitalPinToInterrupt(p) but that macro is not used in Firmata afaik

lets try to get the digitalPinHasPWM(p) and digitalPinToInterrupt(p) macros fixed in esp8266/Arduino

Firmata 2.5 does not compile without VERSION_BLINK_PIN defined

It should compile in 2.5.1 or higher. I removed the dependency on VERSION_BLINK_PIN in v2.5.1.

jnsbyr · 2016-03-18T21:50:23Z

lets try to get the digitalPinHasPWM(p) and digitalPinToInterrupt(p) macros fixed in esp8266/Arduino

Agreed, #define digitalPinHasPWM(p) (((p) < NUM_DIGITAL_PINS)?p:NOT_A_PIN) from
file pins_arduino.h result in the pin list "1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17" for the Huzza and that is not right, cause pin numbering on the ESP starts with 0 and 17 is the analog pin.

Upgraded to Firmata 2.5.2 and now I can do without VERSION_BLINK_PIN.

soundanalogous · 2016-03-20T04:33:39Z

Boards.h

-#define TOTAL_ANALOG_PINS       1
-#define TOTAL_PINS              18 // 11 digital + 1 analog + 6 inaccessible
+#define TOTAL_ANALOG_PINS       NUM_ANALOG_INPUTS
+#define TOTAL_PINS              (NUM_DIGITAL_PINS > A0 + NUM_ANALOG_INPUTS ? NUM_DIGITAL_PINS : A0 + NUM_ANALOG_INPUTS)


the conditional isn't needed anymore, should just be A0 + NUM_ANALOG_INPUTS.

- Firmata 2.5.1 or higher required - esp8266/Adruino needs to fix macros digitalPinHasPWM and digitalPinToInterrupt - no wifio board support

jnsbyr · 2016-03-20T19:35:48Z

Added a request to fix the macros digitalPinHasPWM and digitalPinToInterrupt to issue 1766 of esp8266/Aduino (see esp8266/Arduino#1766 (comment)).

jnsbyr · 2016-04-01T16:12:55Z

The digitalPinHasPWM macro has been fixed in the current Github version and is staged for release with version 2.2.0, see esp8266/Arduino#1831 (comment). This should complete the changes to Boards.h for the ESP8266 variants.

jnsbyr · 2016-04-01T17:48:46Z

Just found something else not considered so far: the ESP8266 project default PWM resolution is 10 bits, not 8, but Firmata always claims 8 bit PWM resolution, even with AVR hardware that can do better (Due, Zero), relying on API defaults. Using analogWriteResolution or analogWriteRange the resolution may be modified. The behaviour is probably platform depended.

soundanalogous · 2016-04-01T20:17:46Z

I drafted a proposal to add analog configuration a couple of years ago: firmata/protocol#8

I also opened a PR for this long ago but ran into MCU architecture issues that I was not able to resolve: #174

Open to picking this back up if I can find the time.

jnsbyr · 2016-04-02T09:43:22Z

#174 adds interesting configuration options but they go much further than necessary for basic operations. A capability query should not return the wrong properties. Even if a platform does not support changing PWM resolution, it should report the build-in resolution and on more powerful platforms the default resolution.

A simple but ugly way to do it is

        if (IS_PIN_PWM(pin)) {
          Firmata.write(PIN_MODE_PWM);
#if defined(ARDUINO_ARCH_ESP8266)
          Firmata.write(10); // 10 = 10-bit resolution, see Arduino.h PWMRANGE
#else          
          Firmata.write(8);  // 8 = 8-bit resolution
#endif          
        }

but a new property like #define DEFAULT_PWM_RESOLUTION 8 in Boards.h would provide the basic requirements without a protocol enhancement:

        if (IS_PIN_PWM(pin)) {
          Firmata.write(PIN_MODE_PWM);
          Firmata.write(DEFAULT_PWM_RESOLUTION);  // resolution in bits
        }

soundanalogous · 2016-04-02T21:03:43Z

I think the second approach is a good short-term solution. Some Arduino variants already report the default pwm resolution (for example), but it's probably irregular enough to justify adding it to Boards.h. Can you add DEFAULT_PWM_RESOLUTION? Then I think this PR will be ready to merge.

soundanalogous · 2016-04-02T21:05:18Z

Boards.h

-#define IS_PIN_SPI(p)           (false)
+#define TOTAL_ANALOG_PINS       NUM_ANALOG_INPUTS
+#define TOTAL_PINS              A0 + NUM_ANALOG_INPUTS
+#define PIN_SERIAL_RX           3


Do pins 3 and 1 appear to be common for RX and TX across all ESP8266 board variants?

Serial has some special properties with the ESP8266. I don't think they can be completely described in a Boards.h approach (e.g. similar to the PWM resolution you can change the serial pin assignment and there is a Serial1 but you typically can't use the RX pin). The details are described here.

All boards seem to have Serial in common and the default mapping for Serial is always pins 1 and 3.

- default to 8-bit for all architectures and board - ESP8266 default is 10-bit

jnsbyr added 2 commits March 13, 2016 12:15

pin functions based on board depended defaults

cefeaa3

to support different ESP8266 boad layouts, use board depended constants from "...\esp8266\hardware\esp8266\2.1.0\variants\...\pins_arduino.h"

analog pin mapping fixed

fb243ce

requires Arduino core for ESP8266 V2.2

soundanalogous reviewed Mar 13, 2016
View reviewed changes

VERSION_BLINK_PIN and PIN_SERIAL1_TX removed

a375d3a

rwaldron reviewed Mar 13, 2016
View reviewed changes

jnsbyr mentioned this pull request Mar 15, 2016

analog pin paramter for analogRead not zero based esp8266/Arduino#1766

Closed

most remaining absolte pin numbers/counts replaced by defines/constan…

e6a09e5

…ts from pins_arduino.h

soundanalogous reviewed Mar 20, 2016
View reviewed changes

config optimizations

71af575

- Firmata 2.5.1 or higher required - esp8266/Adruino needs to fix macros digitalPinHasPWM and digitalPinToInterrupt - no wifio board support

jnsbyr mentioned this pull request Mar 29, 2016

pin macro digitalPinHasPWM returns invalid results esp8266/Arduino#1831

Closed

fixed macro IS_PIN_INTERRUPT

1f3527a

soundanalogous reviewed Apr 2, 2016
View reviewed changes

jnsbyr added 2 commits April 3, 2016 17:08

new define DEFAULT_PWM_RESOLUTION

ec29b12

- default to 8-bit for all architectures and board - ESP8266 default is 10-bit

examples updated to use define DEFAULT_PWM_RESOLUTION

625cff9

jnsbyr mentioned this pull request Apr 3, 2016

no version without query with Ethernet Firmata #280

Open

soundanalogous merged commit ee5d5f5 into firmata:esp Apr 3, 2016

jnsbyr mentioned this pull request Apr 10, 2016

WiFiStream variant integration #285

Merged

pin functions based on board depended defaults #278

pin functions based on board depended defaults #278

Conversation

jnsbyr commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jnsbyr commented Mar 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soundanalogous commented Mar 13, 2016

soundanalogous commented Mar 13, 2016

soundanalogous commented Mar 13, 2016

soundanalogous commented Mar 13, 2016

jnsbyr commented Mar 13, 2016

soundanalogous commented Mar 13, 2016

soundanalogous commented Mar 13, 2016

jnsbyr commented Mar 13, 2016

soundanalogous commented Mar 13, 2016

jnsbyr commented Mar 14, 2016

jnsbyr commented Mar 14, 2016

soundanalogous commented Mar 14, 2016

jnsbyr commented Mar 14, 2016

soundanalogous commented Mar 14, 2016

igrr commented Mar 15, 2016

rwaldron commented Mar 15, 2016

jnsbyr commented Mar 18, 2016

soundanalogous commented Mar 18, 2016

jnsbyr commented Mar 18, 2016

Choose a reason for hiding this comment

jnsbyr commented Mar 20, 2016

jnsbyr commented Apr 1, 2016

jnsbyr commented Apr 1, 2016

soundanalogous commented Apr 1, 2016

jnsbyr commented Apr 2, 2016

soundanalogous commented Apr 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment