namespace firmata #348

zfields · 2017-02-24T02:37:28Z

Continues to expose the API that was previously exposed, but also allows a consumer to use the complete API fully encapsulated in the firmata namespace

Continues to expose the API that was previously exposed, but also allows a consumer to use the complete API fully encapsulated in the `firmata` namespace

zfields · 2017-02-24T02:38:33Z

This allows firmata to pulled into bigger projects and ensures there will be no name collisions. This should be a no-op but it would still be good to test.

zfields · 2017-02-24T03:29:57Z

I just confirmed it passes firmata_test on an Uno and it runs with the ESP8266 node sample.

zfields · 2017-02-24T03:33:12Z

I initially did this for my project, but I just realized this addresses issue #253

However this approach does not break your API, and therefore does not need to wait for v3.0. However, you are free to eliminate everything not in a namespace at v3.0 or as you see fit.

soundanalogous · 2017-02-24T05:25:16Z

FirmataConstants.h

-#undef SYSEX_REALTIME
-#endif
-#define SYSEX_REALTIME          0x7F // MIDI Reserved for realtime messages
+static const int SERIAL_MESSAGE =          0x60; // communicate with serial devices, including other boards


do these each consume 16 bits?

#defines are known as integer literals and treated as int so this shouldn't make any difference. Also both the #defines and static const will show up in the .data section of the memory map.

To confirm I ran side-by-side compiles looking at the "program storage space" and "dynamic memory". Neither change for the Uno, 101^ or ESP8266.
^101 only reports "program storage space"

Yes, but they always did. #defines in the context above are also known as integer constants. Both static const int x = 0x7F; and #define 0x7F require the same amount of space in the .data segment.

In a side-by-side compile, there is no change in the "program storage space" or "dynamic memory" usage for the Uno, 101* and ESP8266.

*The 101 only reports "program storage space".

soundanalogous · 2017-02-24T05:26:31Z

Firmata.h


 #include "Boards.h"  /* Hardware Abstraction Layer + Wiring/Arduino */
-#include "FirmataConstants.h"
+#include "FirmataDefines.h"


I don't think this filename change was intentional.

oh nevermind... I see you split FimrataConstants into 2 files.

This split allows big project users to have all the constants under the firmata namespace, while Arduino users get to keep the global preprocessor defines.

The global preprocessor defines are based off the constants, so there is a single point of reference. Also, the clean separation of the defines, allows them to be removed at a later date (v3.0) if desired.

soundanalogous · 2017-02-24T05:28:15Z

Firmata.h

-    void attach(uint8_t command, ::systemCallbackFunction newFunction);
-    void attach(uint8_t command, ::stringCallbackFunction newFunction);
-    void attach(uint8_t command, ::sysexCallbackFunction newFunction);
+    void attach(uint8_t command, callbackFunction newFunction);


when to use :: ?

When referring to Firmata components, the less we can use :: the better, because it points to the global namespace (which is what we are trying to get away from).

I tucked the callback types into FirmataClass (as they are related to its implementation), and now I am referencing them there. A good example of why this is important is a name like systemCallbackFunction, which can be considered ambiguous.

I updated the pre-existing globals to be based on these new, scoped types so they can be referenced by existing sketches, but can be easily removed in the future (v3.0).

soundanalogous · 2017-02-24T05:29:25Z

Do all of these new declarations consume memory?

zfields · 2017-02-24T12:57:39Z

There should be no additional run-time tax for these declarations. They are only duplicate type definitions (which amount to text), and will be optimized away by the compiler.

Which there seems to be evidence of, especially when compiling side-by-side.

soundanalogous · 2017-02-24T21:31:10Z

FirmataConstants.h

-#define FIRMATA_FIRMWARE_MAJOR_VERSION  2
-#define FIRMATA_FIRMWARE_MINOR_VERSION  5
-#define FIRMATA_FIRMWARE_BUGFIX_VERSION 4
+static const int FIRMATA_FIRMWARE_MAJOR_VERSION =  2;


remove FIRMATA_ from this since it's no longer necessary due to the namespace (we'll need to keep it in the define however)

soundanalogous · 2017-02-24T21:31:20Z

FirmataConstants.h

-#define FIRMATA_PROTOCOL_MAJOR_VERSION  2 // for non-compatible changes
-#define FIRMATA_PROTOCOL_MINOR_VERSION  5 // for backwards compatible changes
-#define FIRMATA_PROTOCOL_BUGFIX_VERSION 1 // for bugfix releases
+static const int FIRMATA_PROTOCOL_MAJOR_VERSION =  2; // for non-compatible changes


remove FIRMATA_

soundanalogous · 2017-02-24T21:33:37Z

FirmataConstants.h

-#undef SYSEX_REALTIME
-#endif
-#define SYSEX_REALTIME          0x7F // MIDI Reserved for realtime messages
+static const int SERIAL_MESSAGE =          0x60; // communicate with serial devices, including other boards


This would also be a good time to align on terminology. I've been debating between _MESSAGE and _DATA and unifying. Traditionally it's been _DATA, but when I added Serial I thought _MESSAGE may make more sense since that's a midi term. I need to think about this some more.

As soon as you figure out how you want this handled, I will be happy to switch it up. Also happy to have this in place and be able to code against it!

IMO, everything adhering to a protocol is a message, and when you have bytes outside the protocol they are data. I think of data as bytes that pass through a transport (i.e. Serial). Don't know if that provides any clarity, but thats what you made me think of.

Also, you can always pull these changes, and the namespaced names can be changed at any time before you stamp it with a version.

@soundanalogous After reading the MIDI spec this morning, I'm going to take another stab at this. I think of the framing bytes and its payload as a (midi) message. While it is true the data encapsulated by the Firmata protocol is indeed a serial message, the actual description of the payload itself is serial data. That being said, my vote is for SERIAL_DATA.

SGTM and keeps requires far fewer changes moving forward. I'll update in the protocol documentation as well.

I've updated SERIAL_MESSAGE to SERIAL_DATA. I think DIGITAL_MESSAGE and ANALOG_MESSAGE are named correctly. Are there any others we should change while we are doing this?

that's all I can think of for now

soundanalogous · 2017-02-26T06:12:48Z

Ran this through all of my tests with no issues. @zfields good to go unless there is anything else you wanted to add here.

zfields · 2017-02-26T08:34:18Z

@soundanalogous I am ready for you to pull. The only thing not secured by the firmata namespace are the contents of Boards.h. However Boards.h is a very Firmata server (Arduino-side) specific file and provides no value (as I can see) to anyone wishing to use this a Firmata client library.

namespace firmata

1819548

Continues to expose the API that was previously exposed, but also allows a consumer to use the complete API fully encapsulated in the `firmata` namespace

soundanalogous reviewed Feb 24, 2017

View reviewed changes

Remove redundant naming

6004516

zfields force-pushed the namespace branch from ac16f7a to 6004516 Compare February 25, 2017 18:11

Unify sysex payload naming to DATA

3cd149c

zfields force-pushed the namespace branch from 42ed053 to 3cd149c Compare February 26, 2017 04:13

soundanalogous merged commit bf3f08f into firmata:master Feb 26, 2017

zfields deleted the namespace branch February 26, 2017 19:11

namespace firmata #348

namespace firmata #348

Uh oh!

Conversation

zfields commented Feb 24, 2017

Uh oh!

zfields commented Feb 24, 2017

Uh oh!

zfields commented Feb 24, 2017

Uh oh!

zfields commented Feb 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soundanalogous Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soundanalogous commented Feb 24, 2017

Uh oh!

zfields commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

zfields Feb 25, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

soundanalogous commented Feb 26, 2017

Uh oh!

zfields commented Feb 26, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

zfields Feb 24, 2017 •

edited

Loading

zfields Feb 24, 2017 •

edited

Loading

soundanalogous Feb 24, 2017 •

edited

Loading

zfields Feb 24, 2017 •

edited

Loading

zfields commented Feb 24, 2017 •

edited

Loading

zfields Feb 25, 2017 •

edited

Loading

zfields Feb 25, 2017 •

edited

Loading

zfields Feb 25, 2017 •

edited

Loading