Skip to content

Moving Firmata constants into separate file to allow direct linking in C projects #213

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

Closed
wants to merge 2 commits into from

Conversation

evanlooij
Copy link

I'm working with ADArduino and iFirmata client libraries and found that each used its own (version of) the Firmata constants. Including Firmata.h doesn't work because of its dependencies on other arduino classes which have no place in a client library. The easiest solution is to move the constants into their own header file. To facilitate modular checking out and inclusion by way of Subversion externals, the Firmata_Constants header should be in its own folder.
To clarify, the structure that I'm proposing is:

arduino

  • constants
    • Firmata_Constants.h
  • Firmata.h (#import "Firmata_Constants.h")
  • Firmata.cpp (no changes)
  • ...

…enable developers of client libraries to link against the original constants used in the protocol. Placing Firmata_Constants.h in the constants folder enables Subversion users to reference Firmata_Constants.h in an external.
…enable developers of client libraries to link against the original constants used in the protocol. Placing Firmata_Constants.h in the constants folder enables Subversion users to reference Firmata_Constants.h in an external.
@soundanalogous
Copy link
Member

Please provide more detail. I don't understand what you are trying to accomplish here as in why I should change Firmata to conform to a client library. Ideally the relationship should be the other way around.

@evanlooij
Copy link
Author

My client library makes extensive use of the Firmata constants: every request is build with them, every response is parsed with their help. However, I cannot just checkout or clone these constants from GitHub to include them in my library, confident that I have the correct constants, for whatever branch or tag I'm working on.
In the present setup, client libraries have to make their own copies and versions of those constants, with all the room for error that entails. By moving the constants into their own file, everybody can be singing from the same sheet, as it were. Client libraries can then, of course, redefine constants or make enums as they wish, but the source would be clear.

firmata_constants 2

@DomAmato
Copy link

I would be tempted to support a separate header file but I don't know that your reason for your request would be useful beyond C based languages and many firmata clients are not written in C. In fact most of the more robust and up to date ones are not so including a language specific header file is probably not ideal.

I think a stronger argument would be to separate the constants into a separate header file similar to the way the boards.h is for organizational purposes. To me that makes more sense than for the occasional C/C++ client to include the header. There is also a problem with your request since the defines may redefine certain system definitions and so having each client set the naming conventions of the constants would actually be better off to prevent such issues.

@soundanalogous
Copy link
Member

Another issue is that the constants.h file could be updated more frequently than the client implementation. This would leave version numbers (if all defines are included) and total number of pin modes out of sync with the actual implementation.

One thing I am open to and have been thinking about is moving some constants (these specifically) to a separate features.h file, but other constants would remain in Firmata.h.

@evanlooij
Copy link
Author

I'd be happy with a more language-neutral solution, a separate c header was what came to mind as a quick and easy fix. Basically, I'm hoping for something that would function as an API.

Too few C libraries: Ruby can be easily extended with C and .NET, iOS and OSX can all use the same headers. That there aren't many and/or good C-based clients is all the more reason to provide them with the proper tools to get going. Arduino may be open source, but it has an introverted feel and is very hard to program against. The guy who developed EmbedXcode spend 800+ hours on it and what does EmbedXcode do? It compiles Arduino.

Contents of constants.h:

  • @soundanalogous: I hadn't realized it, but you are right that FIRMATA_MAJOR_VERSION, FIRMATA_MINOR_VERSION and FIRMATA_BUGFIX_VERSION should not be in constants.h at all, but in Firmata.h: functionally, they are variables rather than constants.
  • Ideally, only message commands or functions should be in constants.h. Why that should exclude the message command bytes like START_SYSEX is not clear to me.
  • Ideally, the functions now contained in Boards.h should be accessible to clients as well.
  • Might constants.h not be a an ambiguous name, conflicting with the thousands of constants.h already out there? Cloning 'firmata' and ending up with a folder named 'arduino' is already a source of conflicts and confusion.

@soundanalogous
Copy link
Member

What might be better is to remove all Arduino specific code from Firmata.cpp and make it a general purpose C++ library. Then Firmata.h, Firmata.cpp and FirmataFeatures.h (what you're calling Firmata_Constants) would work with any c-based client as is. The project is currently called /firmata/arduino/ because it is the Arduino implementation of the Firmata protocol. There is currently no general purpose C/C++ version. Want to take the lead on that? I may not have time for a while.

@soundanalogous
Copy link
Member

Created a new issue to track what I think is the ideal solution here: #215.

@soundanalogous
Copy link
Member

This is being taken care of in: #319

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.

3 participants