Please set default pin mode to INPUT on system reset

[gkzsolt]

In StandardFirmata.ino, systemResetCallback() sets the default mode to OUTPUT (and value LOW) for all pins on startup. This will short-circuit the pins which are connected to HIGH logic level by other circuits, causing malfunctioning or serious damage. The Arduino itself defaults all pins to INPUT mode on power up, which physically can not do any damage and is the only safe mode in any hardware situation. Please follow this and set the pins mode to INPUT in the sketch as well.

[soundanalogous]

It's not this simple for a couple of reasons:

1. Changing to INPUT will introduce a new problem. If pull-up resistors are not enabled for all digital pins, then random pin states will be reported continuously since StandardFirmata checks for pin state changes on each iteration of the main loop. I'm not going to rely on users to attach physical pull-ups to every unused digital pin. I could use INPUT_PULLUP, but that is only an option if every board (and there are many, including non-Arduino boards as well) compatible with StandardFirmata has internal pull-up resistors for every digital pin (that will take some time and research to determine).
2. Legacy. StandardFirmata has set all pins to OUTPUT since it's inception in 2007 or 2008 so users applications currently rely on this state. Obviously not a reason not to make a change, but it will affect existing applications if users update to a new version of StandardFirmata with this behavior.

This issue has come up a couple of times in recent months so I have been thinking about it. The best approach seems to be either:

1. Changing to use INPUT_PULLUP (but again this only works if all supported boards have internal pull ups for all digital pins) and releasing Firmata 3.0 along with other breaking changes.
2. Providing a config.h file with the StandardFirmata sketch that enables the user to customize the pin states at startup. Personally I think this is the best approach.

[soundanalogous]

Another option that gets into Firmata 3.0 territory would be to default all digital pins to INPUT, but add a new message to enable reporting of digital pins on a per pin basis (currently it's per port - which is a 8 pin collection, not necessarily directly in line with a physical microcontroller port). Then in the main loop, only report pin changes for those pins that have been enabled. This is currently how analog pin reporting works so it would make sense to align them.

[gkzsolt]

I am thinking along a few principles. Setting all pins to output can not be a solution, because it is unacceptable from an electronic point of view. I cannot imagine a properly designed circuit with INPUT controller pins which starts by setting the INPUT pins to OUTPUT. This is very intruding, unnecessarily and in a bad way. Legacy users may have coped somehow with this situation, but it is still a big mistake, made by design. This must be corrected.

The reporting of random pin states in the main loop if pins are in INPUT mode is also a Firmata problem. You’re right in not assuming that all users will attach physical pull-ups to every unused digital pin, but you can - and should - assume they will attach proper pull-ups or circuitry for the used INPUT pins. And reporting pin states makes sense only for used pins, isn’t it? It is reasonable to assume that users wishing to read the state of a pin will give some information about this before.

The usual workflow, without Firmata is:

pinMode( pin, INPUT )
digitalRead( pin )

So one could record reporting of a digital pin while doing the setPinMode callback, or something similar. Only report pin changes for those pins that have been enabled, as you said in the last comment (it’s true I don’t know how to set reporting for a pin, the API is somehow not too easy to follow).

Changing to use INPUT_PULLUP for all pins and perhaps all boards I don’t think is a good idea or necessary (again, although it’s not damaging, but it is intrusive, and not necessary.)

Providing a config.h file with the StandardFirmata sketch that enables the user to customize the pin states at startup.

Well, I don’t know. A lot of people are told to load only the StandardFirmata sketch on the Arduino and they’re done. Cannot they bother with setting the pin modes from the client? And the sketch to do only what is necessary, and not be too smart.

The documentation also needs some improvement, at least to tell about the pin modes and states on startup, what the sketches actually do. StandardFirmata is loaded many times.

[soundanalogous]

but you can - and should - assume they will attach proper pull-ups or circuitry for the used INPUT pins

edit I had misread your quote the first time... I thought I had read "... for the unused INPUT pins...", yes, I totally agree that users will attach proper pull-ups or circuitry for the used INPUT pins otherwise they wouldn't get very far :).

My assumption is that a large number of users of StandardFirmata know very little about electronics and are likely piecing together circuits based on Fritzing diagrams. They are mainly using Firmata via a client library in MaxMSP, Processing, JavaScript, Python, etc and the extent of their knowledge of StandardFirmata is whatever the documentation for that library provides. To those users StandardFirmata is just some sketch a client library documentation instructed them to install and for the most part forget about.

That said it's true Firmata is lacking in documentation and better Firmata documentation would help client library authors provide better documentation as well. It's one of the many things on my list. One thing that would really help is if more people would contribute to Firmata, that would help free up some of my very limited time for focusing on things like documentation. It's an open source project, anyone is free to pick up an existing issue and/or make a proposal for a new feature, fix a bug or whatever. Anyway I digress...

The usual workflow, without Firmata is:

pinMode( pin, INPUT )
digitalRead( pin )

This is true, but StandardFirmata differs here in that there is no real concept of digitalRead or analogRead in that the user requests the state of a digital or analog pin like they do in an Arduino sketch and immediately receives a value in return. In StandardFirmata you enable and disable reporting of digital ports (rather than individual pins) and analog pins and that reporting continues until stopped by the user. This design was to reduce the latency at least for digital port reporting. The digital port reporting is one of the reasons the default OUTPUT pinMode has remained unchanged for so many years. I'm open to changing this.

I like your idea for having pinMode indicate that reporting should be enabled for a specific pin. However since in Firmata reading a digital pin is really a concept of reporting the change in state of a digital pin, we'd need a way to stop reporting as well. pinMode(pin, OUTPUT) could be used to stop reporting, but that would just return us to the issue at hand here. Therefore I think a better approach as I mentioned in a previous comment is to add a REPORT_DIGITAL_PIN message to the firmata protocol. This could be super simple such as:

0  START_SYSEX            (0xF0)
1  REPORT_DIGITAL_PIN     (0x63)
2  pinNumber              (0 - 127)
3  reportingEnabled       (0 | 1)
4  END_SYSEX              (0xF7)

The client application could send this message along with the SET_PIN_MODE message to simplify the functionality, but I'm not sure if I'd want StandardFirmata to automatically enable pin reporting when the pinMode is set to INPUT or INPUT_PULLUP. I think that may be better left to the user to decide.

The implementation may be somewhat tricky as everything is currently port based. However, I'm not sure how important port-based reporting is if reporting becomes available at the individual pin level. If a user needs to receive updates on the state of several pins simultaneously, then port reporting has an advantage, but if it's only a few pins and perhaps that are not even in the same "port" then there is really no performance and latency improvement in port reporting. The existing SET_DIGITAL_PIN_VALUE message could be used for this purpose. It's currently only used by clients to set a pin value on the board, but could be used in the other direction as well.

[gkzsolt]

I am happy that we are, AFAIK, on the same track ;) Sorry if I was strongly critical, I just put my thoughts down.

My assumption is that a large number of users of StandardFirmata know very little about electronics and are likely piecing together circuits based on Fritzing diagrams. They are mainly using Firmata via a client library in MaxMSP, Processing, JavaScript, Python, etc and the extent of their knowledge of StandardFirmata is whatever the documentation for that library provides. To those users StandardFirmata is just some sketch a client library documentation instructed them to install and for the most part forget about.

Surely you are right, and then there are the users who are going to use a client library, but first do a glance on the Firmata sketch and (perhaps) its GitHub page, to know what the sketch is doing and then the client library developers. (I am in the middle and would have been grateful to know apriori that the sketch will put the pins into OUTPUT LOW on start.) That said, I am happy to help with adding a few bits to it.

As for the solution, I think we agree that the best is to report only those digital pin - or port - states who are registered for reporting. As I understood from the code, currently this is done so for analog pins, while for the rest - which means all digital pins - all are read and any change is reported. This would change to read and report change only for pins (or ports) registered by a digital reporting command.

That’s what I thought so far. Now, looking at the code I am not sure. It seems to me that digital ports are checked/reported only if reporting is enabled:

void checkDigitalInputs(void) { if (TOTAL_PORTS > 0 && reportPINs[0]) outputPort(0, readPort(0, portConfigInputs[0]), false); // ... }

And (correctly) they are not enabled by default/reset, so need a client command. That’s correct.

It means also, if I am not mistaken, that

If pull-up resistors are not enabled for all digital pins, then random pin states will be reported continuously since StandardFirmata checks for pin state changes on each iteration of the main loop.

will not hold. So one can set the default pin modes to INPUT, without fear of noise through the communication channel.

I have a question here, btw. How can the client know the real state of a pin, or pins/ports ? This should be sent at least at startup, to have a reference by which to infere a pin’s real state.

I like your design of the REPORT_DIGITAL_PIN command. The users will definitely ask for pins, although they will be using a library implementation (also I don’t see too much the need to stop reporting, but that’s a detail). Regarding the implementation, for more performance I think that ideally the software would decide how to optimize reading, which could be port based. What do you think ?

[soundanalogous]

It seems to me that digital ports are checked/reported only if reporting is enabled

This is correct, a digital port is only reported if that port is enabled (all are disabled by default) and at least one pin in that port has changed state since the last check. Adding REPORT_DIGITAL_PIN will change this so that a digital port will be reported only if any pins in that port are enabled and at least one of the enabled pins has changed state. This also requires the client library to check the pins against the previous state to only report the changed pins to the user (since all enabled pins in a port will be reported even if not all enabled pins changed since the last DIGITAL_MESSAGE was received).

One advantage of using SET_DIGITAL_PIN_VALUE instead of DIGITAL_MESSAGE (port-based) is the the client application no longer has to track pin state, but of course the disadvantage is it requires sending more messages when several pins are being reported. I'm still leaning towards using DIGITAL_MESSAGE, at least for the first iteration.

[gkzsolt]

OK, so I think the solution would be:

1. Set the default mode of all digital pins to INPUT (not INPUT_PULLUP)
2. Add a new REPORT_DIGITAL_PIN command to enable reporting of digital pins on a per pin basis. This is a solution you already outlined in a previous comment. The DIGITAL_MESSAGE seems to be not usable correctly here.

[soundanalogous]

Adding REPORT_DIGITAL_PIN will be the first step since this can safely be done in master without having any impact on existing user applications. I'll have to create a new branch to change OUTPUT to INPUT and then work with Firmata client authors on a rollout plan or it will break all existing user applications. This is complicated by the fact that Arduino pulls Firmata master into their library bundle before cutting a new IDE release.

[soundanalogous]

https://github.com/firmata/protocol/issues/68

[odewdney]

Could the default digital pin be INPUT_NOT_REPORTING, and if the user sets it to INPUT, it becomes reporting, so little change on the client side.

[soundanalogous]

Yes, that is the plan but it's not so simple because there is currently no sense of individual pin reporting in Firmata, it's controlled at the 8 pin port level. This will be a large refactor and getting the pin reporting protocol right takes time (and ideally a good number of people weighing in on a proposal). The plan is to roll this out with Firmata 3.0, but there is currently no timeline. The best people can do is weigh in on Firmata 3.0 proposals.