separate hardware definition from Firmata Device

[alizelzele]

I am using this library for connecting my android phone to arduino with OTG cable. to do so, I had to change the code to define another type of device instead of fixed serial hardcoded into FirmataDevice implementation. I cleaned up my changes and polished it a bit for anybody who needs it. It is backward compatible and the old constructor is still there. now this library can be used as a core library for communicating with firmata through any platform ( Bluetooth, websocket, RF, ...) let me know if you think additional changes are required.

1. now a custom device can be connected to FirmataDevice
2. adding git ignore for intellij idea files

[kurbatov]

Hi Ali,

Your pull request looks good to me. It addresses the right issue that will be appreciated by many users of the library.

I know that Thomas Welsch (@ttww) is working on that as a part of a bigger effort. His fork contains a little bit different solution to the same issue.

Both solutions inherit a design flaw of the initial implementation of FirmataDevice: byteQueue being the member of FirmataDevice leaks to another class and breaks incapsulation. Untill now it was not so evident because it leaked only to a private nested class (FirmataParser). When transport interface is separated from FirmataDevice, byteQueue leaks to that interface too.

Thomas' solution, to my taste, has a little advantage: it arranges DeviceConnector (matches to FirmataTransportInterface) as an interface and byteQueue leaks outside of the constructor of FirmataDevice. This pull request, on the other hand, is concise and to the point.

I think it would be great to have an integral solution that incorporate advantages of both solutions and gets rid of described design flaw. I would really appreciate if you evolve your solution (in cooperation with @ttww if he agrees to participate in review) so that it is more inline to Thomas' solution.

If it is not possible for you to spend more time on that (which is totally fine and does not reduce significance of your contribution), I will come up with the third solution based on yours and Thomas'.

Thank you for your time and efforts.

[alizelzele]

I admit I implemented the laziest possible way :). By reading the code a bit right now, I saw there is a finiteStateMachine that handles events and request. To be honest, I did not get how it works (I am using github code viewer now). if I can find a bit spare time, I will try to understand the code more, and also check the fork by ttww

-- UPDATE -- @kurbatov checked the code and saw the parser package and states. 👍 checked firamata protochol details here: http://firmata.org/wiki/Protocol likely All messages end with byte END_SYSEX (0xF7). I suggest we move the byteQueue out to common connector implementation and connectors can just write their data in there, then It will fire an event for parser to handle with the bytes and do the magic whenever it saw the end sysex. What do you think about it ?

[alizelzele]

@kurbatov updated the naming and added interface + abstract implementation. next step is to tackle byeQueue leaking.

[kurbatov]

@alizelzele, I like the idea that every connector handles datastream on its own. However the data should be passed to the parser in the fastest possible way.

I think events from a connector is hard to implement. The thing is: not every message from a board has the same ending (analog and digital messages for example). So connector cannot decide when a message is received not knowing the details of the protocol. Batching data by time is not an option because it slows down the processing. Wrapping every byte to an event is not an option too because it increases memory consumption.

After all maybe sharing a queue as a communication means between a connector and the parser is not such a bad design if arranged properly.

BTW the firmata wiki you referenced to is not actevely maintained. The latest version of the protocol documentation is here.

[alizelzele]

@kurbatov I separate the parser definition and update code to handle it in the common transport itself. I don't have access to Arduino at the moment to test the changes. Can I have your opinion on the new implementation?

[kurbatov]

Ali, thank you for your contribution.

[alizelzele]

@kurbatov Hope you have tested it. I did not have access to Arduino when writing it to test it and have not test it yet :rofl: