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

Implement new feature DeviceFirmata. #52

Open
wants to merge 39 commits into
base: master
Choose a base branch
from

Conversation

finson
Copy link

@finson finson commented Jun 7, 2016

The changes in this pull request implement a new DeviceDriver feature for Configurable Firmata. In addition to these changes, there is a separate Arduino library named Luni, and a Javascript client library LuniJS. The repositories for these packages are available at finson-release.

There are specification documents in the Luni library repo that define the interface between Firmata and the device drivers.

The following drawing shows the relationship between the various classes and modules involved. This drawing is also available in the LuniJS repo.

luni architecture

…e-firmata

# Conflicts: (versioning conflicts, resolved)
#	library.properties
#	src/ConfigurableFirmata.h
	src/ConfigurableFirmata.h
@finson
Copy link
Author

finson commented Jun 8, 2016

Jeff: I've been using 2 Leonardos and an Arduino Micro for testing.

I'll get you some memory numbers. Experience wise, I've generally been able to run 2 drivers when I leave out most of the configurable features.

Despite the long list of commits there are only changes to five files in ConfigurableFirmata: DeviceFirmata<.cpp, .h>, the .ino, the associated SelectedDevices.h, and the change to ConfigurableFirmata.h to add the Sysex codes.

A good review is definitely in order! The devil is in the details.

@finson
Copy link
Author

finson commented Jun 8, 2016

Here are the results of some memory testing.

v0.8 memory usage tests

@finson
Copy link
Author

finson commented Jun 13, 2016

Jeff: FYI. I find that because of the way that Arduino does the build process, there is a fair amount of junk left behind by the files that are compiled, linked, then removed. So I did an experiment:

  • Run 0 is ConfigurableFirmata with only DeviceFirmata selected in the .ino file, but all the other features still in the src directory as usual.
  • Run 1 did not change any code, but I stashed all the unused feature files in a directory outside of the src directory. You can see that it reduced the initial use of dynamic storage by 262 bytes.

memory-usage


#include "DeviceFirmata.h"
#include "utility/Boards.h"
#include <Base64.h>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the advantage of using Base64 encoding rather than 7-bit encoding with the existing Encoder7Bit class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My assumption is availability of Base64 libraries for client applications, but wanted to understand if there are other reasons/advantages.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Client availability.
Documentation and knowledge widely available.
No embedded Firmata calls. I'm trying to make sure that Luni can be used
totally standalone, or perhaps with other comms protocols, and I didn't
want anything that assumed the Firmata environment.

On Sun, 19 Jun 2016 23:07:42 -0700, Jeff Hoefs notifications@github.com
wrote:

In src/DeviceFirmata.cpp:

@@ -0,0 +1,264 @@
+/*

  • DeviceFirmata.cpp - Firmata library
    +*/

+#include "DeviceFirmata.h"
+#include "utility/Boards.h"
+#include <Base64.h>

My assumption is availability of Base64 libraries for client
applications, but wanted to understand if there are other
reasons/advantages.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jeff:

I wanted to clearly separate the comms codec and make sure that nothing
related to it leaked out into the Luni code or the user client code.
Using a standard codec and applying it right at the point of send and
receive seemed like a good way to achieve that.

I wrote a Firmata client library in Java to support the early design and
testing, and it was a one-line task to do the Base64 work, and later I had
the same experience in Javascript. So I left the redundancy of the codecs
in the design.

Doug
Client availability.
Documentation and knowledge widely available.
No embedded Firmata calls. I'm trying to make sure that Luni can be used
totally standalone, or perhaps with other comms protocols, and I didn't
want anything that assumed the Firmata environment.

On Sun, 19 Jun 2016 23:07:42 -0700, Jeff Hoefs notifications@github.com
wrote:

In src/DeviceFirmata.cpp:

@@ -0,0 +1,264 @@
+/*

  • DeviceFirmata.cpp - Firmata library
    +*/

+#include "DeviceFirmata.h"
+#include "utility/Boards.h"
+#include <Base64.h>

My assumption is availability of Base64 libraries for client
applications, but wanted to understand if there are other
reasons/advantages.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

@soundanalogous
Copy link
Member

Is it necessary that an old version of Boards.h is included with the Luni library?

@soundanalogous
Copy link
Member

Looking over the Planned and Deferred changes in LuniLib it appears that the longer term strategy is to provided general control over the microcontroller (GPIO, Wire, SPI, OneWire). How does this map out against Firmata or is ConfigurableFirmata a temporary stepping stone on a larger roadmap (which is fine by me BTW, just looking for clarity into how long lived Firmata integration would be).

@soundanalogous
Copy link
Member

I'm assuming the src-drivers are either not included (or the compiler optimizes them away) if LuniLib is included but those particular drivers (Servo, MCP9808, etc) are not used, correct?

@finson
Copy link
Author

finson commented Jun 20, 2016

Edited to add: This conversation about boards.h became issue finson/Luni#6.

No, in fact I'd like a simpler way to access Boards.h without having to
fold Luni into Firmata entirely. I am using the macros and other
definitions in there as I try to make it fit with Firmata better.
Suggestions? The problem is that I want to be able to use Luni as a
standalone approach to writing Arduino code, as well as a FirmataFeature.
But I just couldn't stay away from your hardware abstraction layer!

@soundanalogous
Copy link
Member

Regarding Boards.h, I'd have to look over the macros from that file you are using and whether or not all supported boards already provide equivalent macros and constants in their respective pin variant definitions. It may be possible to avoid the dependency. There are a number of macros that could be removed from Boards.h because standard definitions exist for all Arduino boards (this was not the case years ago, but has improved).

@finson
Copy link
Author

finson commented Jun 20, 2016

Right, they are not compiled at all, and therefore don't contribute
anything to the memory usage. Notice that I've done the same thing with
my version of Configurable Firmata (FirmataWithDeviceFeature). The reason
is that Arduino compiles everything it can see, links it all together,
then has the linker remove the unneeded stuff. This is nice, but it turns
out that there are a number of sections of data that are overlooked during
the removal and so the final memory usage is greater than it should be.
If I remember right, I recovered something like 260 bytes of dynamic
memory by excluding the source files altogether.

On Sun, 19 Jun 2016 23:57:39 -0700, Jeff Hoefs notifications@github.com
wrote:

I'm assuming the src-drivers are either not included (or the compiler
optimizes them away) if LuniLib is included but those particular drivers
(Servo, >MCP9808, etc) are not used, correct?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

@finson
Copy link
Author

finson commented Jun 20, 2016

I didn't use the boards.h macros at first. Partly because there was just
a lot of code to design and write, and partly because I was hoping to
stick to the pin variant files as you suggest. However, in writing the
newest driver, DDSignal, I think I've finally gotten a better
understanding of what is actually needed to make the HAL work and also to
provide feedback to the client about "can some module on the Arduino take
responsibility for activities classified with this mode?". The macros in
boards.h do this pretty well. So I threw in the towel and started using
them in DDSignal. So far, I'm just using them to manage the
analog/digital pin number mapping.

It might be worth thinking about a way to separate pin modes and available
modules. For anything other than Input, Output, and Input_Pullup, they
are not really electrical modes, but rather operational capabilities. My
understanding of Firmata's set pin mode is that it's an opportunity for
the relevant feature to accept responsibility for future transactions, and
also to do whatever initialization it wants to with the pin, with its own
record keeping, or anything else.

On Mon, 20 Jun 2016 00:10:31 -0700, Jeff Hoefs notifications@github.com
wrote:

Regarding Boards.h, I'd have to look over the macros from that file you
are using and whether or not all supported boards already provide
equivalent >macros and constants in their respective pin variant
definitions. It may be possible to avoid the dependency. There are a
number of macros that could be >removed from Boards.h because standard
definitions exist for all Arduino boards (this was not the case years
ago, but has improved).


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

@finson
Copy link
Author

finson commented Jun 20, 2016

My intention is that the integration with Firmata be long lasting. If I
think of Firmata as an existing client side framework with powerful client
libraries and lots of previous work on priorities, hardware abstraction,
and so on, it seems like the device drivers can fill a niche for those
applications where one particular feature really has to be implemented all
on the Arduino. It should easily co-exist with "normal" Firmata code, so
that people can continue to do quick development on the client side. I
think of a device driver as a specialized tool that solves a particular
problem that is hard to solve completely from the client side.

I have a PING sensor sitting right next to me, waiting for the moment when
the Device Driver architecture and integration with Firmata are
"complete", and now it's time for a real and more challenging device.

In the list you quote, GPIO is in fact a device driver (now named
DDSignal). It's mostly intended to be a teaching tool for me to force me
to think about Firmata integration at the signal level, and also to
provide at least one device driver that can actually do something useful.
.

SPI and OneWire in my mind are more in the mold of the I2E code in the
Silicon folder. They are a way for me to collect the specifics of the
particular hardware protocol and make them available to any device driver
author. I thought about trying to integrate these with the related
Firmata features, but I felt that the features were pretty tightly
integrated with Firmata and wouldn't work for Luni. At the beginning I
did in fact do a draft rewrite of I2EFirmata to use my I2E code as a lower
layer, but it was a distraction and unnecessary, so I slid the whole issue
to Planned and Deferred.

On Sun, 19 Jun 2016 23:34:06 -0700, Jeff Hoefs notifications@github.com
wrote:

Looking over the Planned and Deferred changes in LuniLib it appears that
the longer term strategy is to provided general control over the
microcontroller >(GPIO, Wire, SPI, OneWire). How does this map out
against Firmata or is ConfigurableFirmata a temporary stepping stone on
a larger roadmap (which is >fine by me BTW, just looking for clarity
into how long lived Firmata integration would be).


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

@soundanalogous
Copy link
Member

soundanalogous commented Jun 20, 2016

I'm having issues compiling the ConfigurableFirmataDeviceDriver example. I'll have more input tonight when I'm home from work and can look into it further, but I believe part of the issue is that Arduino doesn't compile anything that isn't in the src directory of a library (LuniLib in this case) when importing that library into a sketch. So when including LuniLib in a sketch, the classes in src-drivers are not accessible to the sketch. This is a frustrating behavior of the way Arduino compiles libraries (it walks only the src directory tree and compiles all cpp files it finds). The only hacky solution I've found is to include all optional classes within a subdirectory the src folder and move all of the code for a class to the header file, keeping the cpp file empty or non-existent. You can see examples here (all code is in header files): https://github.com/firmata/arduino/tree/master/utility. The only way around it is to write a makefile. It's a tradeoff Arduino took in order to make the compilation process as simple as possible for newbies.

@finson
Copy link
Author

finson commented Jun 20, 2016

Jeff:

I intentionally moved all the unused features (ConfigFirmata) and all the unused device drivers (Luni) out of the src directories because it saves memory at runtime to not have them compiled, linked, and removed. The easiest solution (at the cost of a little dynamic memory) is to drag all the feature files back to the src dir and all the Luni driver directories back to the Luni src dir. Everything should compile then.

Doug

On Jun 20, 2016, at 12:59 PM, Jeff Hoefs notifications@github.com wrote:

I'm having issues compiling the ConfigurableFirmataDeviceDriver example. I'll have more input tonight when I'm home from work and can look into it further, but I believe part of the issue is that Arduino doesn't compile anything that isn't in the src directory of a library (LuniLib in this case) when importing that library into a sketch. So when including LuniLib in a sketch, the classes in src-drivers are not accessible to the sketch. This is a frustrating behavior of the way Arduino compiles libraries (it walks only the src directory tree and compiles all cpp files). The only hacky solution I've found is to include all optional classes within a subdirectory the src folder and move all of the code for a class to the header file, keeping the cpp file empty or non-existent. You can see examples here (all code is in header files): https://github.com/firmata/arduino/tree/master/utility https://github.com/firmata/arduino/tree/master/utility. The only way around it is to write a linker. It's a tradeoff Arduino took in order to make the compilation process as simple as possible for newbies.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #52 (comment), or mute the thread https://github.com/notifications/unsubscribe/ABONrBeRz5T8ctINB4aXfItyackQv0Bzks5qNvEEgaJpZM4IwU6W.

@finson
Copy link
Author

finson commented Jun 20, 2016

Edited to add: The description below is how it’s supposed to work. However, when I moved everything back just now to confirm it, I realized that I had changed the way the driver subclasses interact with the DeviceDriver base class while writing DDSensor and so it is the only one that still compiles in the current 0.10 branch. So I’ll fix that this afternoon when I can and push a commit for it.

In the meantime, I think that if you move all the Config Firmata features back to src/ and just use DDSensor from Luni, everything should compile again. Fix coming this evening or tonight, depending on how my afternoon shapes up!

Jeff:

I intentionally moved all the unused features (ConfigFirmata) and all the unused device drivers (Luni) out of the src directories because it saves memory at runtime to not have them compiled, linked, and removed. The easiest solution (at the cost of a little dynamic memory) is to drag all the feature files back to the src dir and all the Luni driver directories back to the Luni src dir. Everything should compile then.

Doug

On Jun 20, 2016, at 12:59 PM, Jeff Hoefs <notifications@github.com mailto:notifications@github.com> wrote:

I'm having issues compiling the ConfigurableFirmataDeviceDriver example. I'll have more input tonight when I'm home from work and can look into it further, but I believe part of the issue is that Arduino doesn't compile anything that isn't in the src directory of a library (LuniLib in this case) when importing that library into a sketch. So when including LuniLib in a sketch, the classes in src-drivers are not accessible to the sketch. This is a frustrating behavior of the way Arduino compiles libraries (it walks only the src directory tree and compiles all cpp files). The only hacky solution I've found is to include all optional classes within a subdirectory the src folder and move all of the code for a class to the header file, keeping the cpp file empty or non-existent. You can see examples here (all code is in header files): https://github.com/firmata/arduino/tree/master/utility https://github.com/firmata/arduino/tree/master/utility. The only way around it is to write a linker. It's a tradeoff Arduino took in order to make the compilation process as simple as possible for newbies.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #52 (comment), or mute the thread https://github.com/notifications/unsubscribe/ABONrBeRz5T8ctINB4aXfItyackQv0Bzks5qNvEEgaJpZM4IwU6W.

@finson
Copy link
Author

finson commented Jun 20, 2016

Edited to add: DDSignal, not DDSensor.

Edited to add: The description below is how it’s supposed to work. However, when I moved everything back just now to confirm it, I realized that I had changed the way the driver subclasses interact with the DeviceDriver base class while writing DDSensor and so it is the only one that still compiles in the current 0.10 branch. So I’ll fix that this afternoon when I can and push a commit for it.

In the meantime, I think that if you move all the Config Firmata features back to src/ and just use DDSensor from Luni, everything should compile again. Fix coming this evening or tonight, depending on how my afternoon shapes up!

Jeff:

I intentionally moved all the unused features (ConfigFirmata) and all the unused device drivers (Luni) out of the src directories because it saves memory at runtime to not have them compiled, linked, and removed. The easiest solution (at the cost of a little dynamic memory) is to drag all the feature files back to the src dir and all the Luni driver directories back to the Luni src dir. Everything should compile then.

Doug

On Jun 20, 2016, at 12:59 PM, Jeff Hoefs <notifications@github.com mailto:notifications@github.com> wrote:

I'm having issues compiling the ConfigurableFirmataDeviceDriver example. I'll have more input tonight when I'm home from work and can look into it further, but I believe part of the issue is that Arduino doesn't compile anything that isn't in the src directory of a library (LuniLib in this case) when importing that library into a sketch. So when including LuniLib in a sketch, the classes in src-drivers are not accessible to the sketch. This is a frustrating behavior of the way Arduino compiles libraries (it walks only the src directory tree and compiles all cpp files). The only hacky solution I've found is to include all optional classes within a subdirectory the src folder and move all of the code for a class to the header file, keeping the cpp file empty or non-existent. You can see examples here (all code is in header files): https://github.com/firmata/arduino/tree/master/utility https://github.com/firmata/arduino/tree/master/utility. The only way around it is to write a linker. It's a tradeoff Arduino took in order to make the compilation process as simple as possible for newbies.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub #52 (comment), or mute the thread https://github.com/notifications/unsubscribe/ABONrBeRz5T8ctINB4aXfItyackQv0Bzks5qNvEEgaJpZM4IwU6W.

@soundanalogous
Copy link
Member

Which Base64 library are you using? There are more than one in the Arduino library manager.

@finson
Copy link
Author

finson commented Jun 21, 2016

name=base64
version=1.0.0
author=Adam Rudd
maintainer=Adam Rudd
sentence=A base64 library for the arduino platform, written in C
paragraph=
category=Data Processing
url=https://github.com/adamvr/arduino-base64
architectures=*

On Mon, 20 Jun 2016 16:19:23 -0700, Jeff Hoefs notifications@github.com
wrote:

Which Base64 library are you using? There are more than one in the
Arduino library manager.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

@soundanalogous
Copy link
Member

I have no idea how this is possible, but your fork of ConfigurableFirmata seems to have superseded ConfigurableFirmata in the Arduino Library manager. Anyone who updates apparently gets ConfigurableFirmata v2.9.0, which obviously I've never published. This is a really bad bug on Arduino's part. I'll file an issue with Arduino.

@soundanalogous
Copy link
Member

Update: bug filed: arduino/Arduino#5058

@finson
Copy link
Author

finson commented Jun 21, 2016

Suggestions? I guess I could delete the entire repo from Github. It's
not the master, although it's the only place holding the release tags.
Doug

On Mon, 20 Jun 2016 20:42:57 -0700, Jeff Hoefs notifications@github.com
wrote:

Update: bug filed: arduino/Arduino#5058


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

@soundanalogous
Copy link
Member

According to the library manager FAQ a library can be unpublished by deleting the tag (so 2.9.0) in this case. However I'm not sure which version of your fork caused the issue, if it's the finson one or the finson-release one. Both have release tags. I think it's worth leaving things as is for another day or so hoping that we'll get a response from someone on the Arduino team tomorrow. That will enable them to inspect the current state.

I assume the algorithm that identifies changes in libraries (via an update tag) is not properly factoring in the url of the repo (which would be a unique identifier). There may have been a point when your fork was tagged and in the library.properties file the name wasn't changed yet, then you later submitted a request to have your updated version of the renamed library added to the library manager and that started tracking and the Arduino hourly job picked up the tag somehow and made the unfortunate association based on tag and project name alone. That's my hunch at least.

@finson
Copy link
Author

finson commented Jun 21, 2016

Jeff:

If you think leaving it open for a day or two is okay for debugging
purposes that's fine with me. But let me know if you want me to do
anything (up to and including nuking the repo if necessary!).

I saw the "delete the tag" comment too, but you'll notice that the removal
job only runs every Sunday. So I guess if it's not resolved by Saturday I
can delete the tags and we can start over.

I requested that they add the repo in arduino/Arduino issue #5039. The
repo url in the issue was shown as Repo:
https://github.com/finson-release/FirmataWithDeviceFeature. At the time
when I wrote the request, the library properties file already showed the
changed name (FirmataWithDeviceFeature), so I'm not sure how it happened.
I think your scenario is probably right, but I don't understand the timing
yet. Oh well.

Doug

On Mon, 20 Jun 2016 22:06:58 -0700, Jeff Hoefs notifications@github.com
wrote:

According to the library manager FAQ a library can be unpublished by
deleting the tag (so 2.9.0) in this case. However I'm not sure which
version of >your fork caused the issue, if it's the finson one or the
finson-release one. Both have release tags. I think it's worth leaving
things as is for another day >or so hoping that we'll get a response
from someone on the Arduino team tomorrow. That will enable them to
inspect the current state.

I assume the algorithm that identifies changes in libraries (via an
update tag) is not properly factoring in the url of the repo (which
would be a unique >identifier). There may have been a point when your
fork was tagged and in the library.properties file the name wasn't
changed yet, then you later >submitted a request to have your updated
version of the renamed library added to the library manager and that
started tracking and the Arduino >hourly job picked up the tag somehow
and made the unfortunate association based on tag and project name
alone. That's my hunch at least.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or mute the thread.

Using Opera's mail client: http://www.opera.com/mail/

@soundanalogous
Copy link
Member

Arduino Zero (SAMD architecture) defines an enum named DAC in the global space. This conflicts with the global enum DAC declared here: https://github.com/finson-release/Luni/blob/master/src/Device/DeviceDefine.h#L29, generating a compiler error. It's best to give very specific names to any variables or constants declared in the global scope.

@@ -0,0 +1,264 @@
/*
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of DeviceFirmata.cpp needs to be moved to DeviceFirmata.h or Luni and Bas64 will become hard dependencies of ConfigurableFirmata whether or not the user chooses to use DeviceFirmata.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment became issue finson/issues/9 and was closed with the introduction of Optional Features.

@finson
Copy link
Author

finson commented Jul 14, 2016

This comment on global name space conflict became issue finson/Luni/issues/5.

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

Successfully merging this pull request may close these issues.

None yet

2 participants