Skip to content

Spark #141

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.

Sign up for GitHub

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 5 commits into from
Closed

Spark #141

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
58b3d58
classic strlen instead of sizeof bug closes #105
jacobrosenthal Dec 2, 2013
c04a31c
Revert "classic strlen instead of sizeof bug closes #105"
jacobrosenthal Dec 2, 2013
10168c1
Merge branch 'master' of github.com:firmata/arduino
jacobrosenthal Jul 30, 2014
0893eb5
Spark support
jacobrosenthal Jul 30, 2014
b4ca774
depreciated
jacobrosenthal Sep 6, 2014
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 29 additions & 9 deletions Boards.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,15 @@

#include <inttypes.h>

#ifdef SPARK
#include "application.h"
Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

this include "application" is way too generic. Should be renamed in core project if possible.

Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

i guess it's okay though since it's within the ifdef

Copy link
Contributor

@rwaldron rwaldron Aug 1, 2014

Choose a reason for hiding this comment

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

Arguably this could be part of a SparkFirmata.ino that's tailored to the Spark

Copy link
Member

@soundanalogous soundanalogous Aug 2, 2014

Choose a reason for hiding this comment

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

If the Spark is just another "arduino" then it may make sense to included it with the firmata/arduino stuff. By this I mean if you can already use most Arduino libraries with the Spark, you program it with the Arduino IDE, etc. If that is not the case it may be better to create a separate repo firmata/spark. The reason being that I wouldn't want to maintain exceptions for boards that are only partially Arduino compatible.

Copy link
Member

@soundanalogous soundanalogous Aug 2, 2014

Choose a reason for hiding this comment

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

Also if Firmata.cpp is 100% compatible then simply a separate SparkFirmata.ino could work as well, but we also need to consider the role of configurable firmata not just another variant of StandardFirmata (I finally got around to starting on the firmatabuilder tool).

Copy link
Contributor

@rwaldron rwaldron Aug 2, 2014

Choose a reason for hiding this comment

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

I think that's an issue: there are similar, but Spark-specific versions of all the libraries, I any speak to the compatibility.

Copy link
Contributor

@rwaldron rwaldron Aug 2, 2014

Choose a reason for hiding this comment

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

can't* not "any"

#else
#if defined(ARDUINO) && ARDUINO >= 100
#include "Arduino.h" // for digitalRead, digitalWrite, etc
#include "Arduino.h" // for digitalRead, digitalWrite, etc
#else
#include "WProgram.h"
#endif
#endif

// Normally Servo.h must be included before Firmata.h (which then includes
// this file). If Servo.h wasn't included, this allows the code to still
Expand Down Expand Up @@ -332,6 +336,22 @@ writePort(port, value, bitmask): Write an 8 bit port.
#define PIN_TO_SERVO(p) ((p) - 2)


// Spark
#elif defined(SPARK)
#define TOTAL_ANALOG_PINS 8
#define VERSION_BLINK_PIN 7
#define IS_PIN_DIGITAL(p) (((p) >= 0 && (p) <= 7) || ((p) >= 10 && (p) <= 17))
#define IS_PIN_ANALOG(p) ((p) >= 10 && (p) <= 17)
#define IS_PIN_PWM(p) digitalPinHasPWM(p)
Copy link
Contributor

@rwaldron rwaldron Jul 30, 2014

Choose a reason for hiding this comment

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

This won't work as it does with the other boards defined in this file. The Spark's PWM support is available on D0, D1, A0, A1, A4, A5, A6 and A7.

Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

it will work as long as the spark core firmware defines digitalPinHasPWM

Copy link
Contributor Author

@jacobrosenthal jacobrosenthal Aug 1, 2014

Choose a reason for hiding this comment

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

How about

#define IS_PIN_PWM(p)           ((p) == 0 || (p) == 1 || ((p) >= 10 && (p) <= 17))

Copy link
Member

@soundanalogous soundanalogous Aug 3, 2014

Choose a reason for hiding this comment

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

Ideally the equivalent of an arduino variant pins_arduino.h file should be added to spark/core-firmware. Here's an example from the standard arduino variant: https://github.com/arduino/Arduino/blob/master/hardware/arduino/variants/standard/pins_arduino.h

#define IS_PIN_SERVO(p) ((p) >= 0 && (p) < MAX_SERVOS) //??
Copy link
Contributor

@rwaldron rwaldron Jul 30, 2014

Choose a reason for hiding this comment

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

MAX_SERVOS should equal 8 for the Spark, but then this logic becomes unsound.

Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

This is an issue with Firmata in general. When servo support was first added the number of pins supporting Servo (via the Servo library) was equal to the number of Arduino pins supporting servo. This is no longer the case (and has not been for a few years). MAX_SERVOS is defined in the servo library and is determined by the number of timers available and the number of servos supported by each timer.

Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

for reference: #62

Copy link
Contributor Author

@jacobrosenthal jacobrosenthal Aug 1, 2014

Choose a reason for hiding this comment

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

How about

#define IS_PIN_SERVO(p)         ((p) == 0 || (p) == 1 || ((p) >= 10 && (p) <= 17))

#define IS_PIN_I2C(p) ((p) == 0 || (p) == 1)
#define IS_PIN_SPI(p) ((p) == SS || (p) == MOSI || (p) == MISO || (p) == SCK)
#define PIN_TO_DIGITAL(p) (p)
#define PIN_TO_ANALOG(p) (p)-10
#define PIN_TO_PWM(p) PIN_TO_DIGITAL(p)
#define PIN_TO_SERVO(p) (p)
#define IS_PIN_DISABLED(p) ((p) == 8 || (p) == 9)

// anything else
#else
#error "Please edit Boards.h with a hardware abstraction for this board"
Expand Down Expand Up @@ -402,14 +422,14 @@ static inline unsigned char writePort(byte port, byte value, byte bitmask)
}
#else
byte pin=port*8;
if ((bitmask & 0x01)) digitalWrite(PIN_TO_DIGITAL(pin+0), (value & 0x01));
if ((bitmask & 0x02)) digitalWrite(PIN_TO_DIGITAL(pin+1), (value & 0x02));
if ((bitmask & 0x04)) digitalWrite(PIN_TO_DIGITAL(pin+2), (value & 0x04));
if ((bitmask & 0x08)) digitalWrite(PIN_TO_DIGITAL(pin+3), (value & 0x08));
if ((bitmask & 0x10)) digitalWrite(PIN_TO_DIGITAL(pin+4), (value & 0x10));
if ((bitmask & 0x20)) digitalWrite(PIN_TO_DIGITAL(pin+5), (value & 0x20));
if ((bitmask & 0x40)) digitalWrite(PIN_TO_DIGITAL(pin+6), (value & 0x40));
if ((bitmask & 0x80)) digitalWrite(PIN_TO_DIGITAL(pin+7), (value & 0x80));
if ((bitmask & 0x01)) digitalWrite(PIN_TO_DIGITAL(pin+0), (value & 0x01) == 0x01 ? 1 : 0 );
Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

What is the reasoning behind this change?

Copy link
Contributor Author

@jacobrosenthal jacobrosenthal Aug 1, 2014

Choose a reason for hiding this comment

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

Spark (and maybe others) don't support any random positive number as a truth value. @rwaldron is attempting to push a fix upstream https://github.com/spark/core-firmware/issues/247 so it may not be necessary

if ((bitmask & 0x02)) digitalWrite(PIN_TO_DIGITAL(pin+1), (value & 0x02) == 0x02 ? 1 : 0 );
if ((bitmask & 0x04)) digitalWrite(PIN_TO_DIGITAL(pin+2), (value & 0x04) == 0x04 ? 1 : 0 );
if ((bitmask & 0x08)) digitalWrite(PIN_TO_DIGITAL(pin+3), (value & 0x08) == 0x08 ? 1 : 0 );
if ((bitmask & 0x10)) digitalWrite(PIN_TO_DIGITAL(pin+4), (value & 0x10) == 0x10 ? 1 : 0 );
if ((bitmask & 0x20)) digitalWrite(PIN_TO_DIGITAL(pin+5), (value & 0x20) == 0x20 ? 1 : 0 );
if ((bitmask & 0x40)) digitalWrite(PIN_TO_DIGITAL(pin+6), (value & 0x40) == 0x40 ? 1 : 0 );
if ((bitmask & 0x80)) digitalWrite(PIN_TO_DIGITAL(pin+7), (value & 0x80) == 0x80 ? 1 : 0 );
Copy link
Contributor

@rwaldron rwaldron Jul 30, 2014

Choose a reason for hiding this comment

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

  • The shortened indent needs to be restored
  • I wonder if core-firmware would accept a patch to change this to this:
if (value == LOW)
{
  PIN_MAP[pin].gpio_peripheral->BRR = PIN_MAP[pin].gpio_pin;
} else 
{
  PIN_MAP[pin].gpio_peripheral->BSRR = PIN_MAP[pin].gpio_pin; 
}

That would allow this changeset to be reverted. I've filed this bug: https://github.com/spark/core-firmware/issues/247

Copy link
Contributor

@rwaldron rwaldron Jul 30, 2014

Choose a reason for hiding this comment

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

particle-iot/device-os#248

Copy link
Contributor

@rwaldron rwaldron Aug 3, 2014

Choose a reason for hiding this comment

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

The necessary patch to Spark's core-firmware has landed, so this whole block can be reverted now. particle-iot/device-os@337a86b

#endif
}

Expand Down
7 changes: 5 additions & 2 deletions Firmata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,10 @@
//******************************************************************************

#include "Firmata.h"

#ifndef SPARK
Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

is the SPARK library included in the Arduino core distribution?

Copy link
Contributor Author

@jacobrosenthal jacobrosenthal Aug 1, 2014

Choose a reason for hiding this comment

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

Spark is a compiler define, not unlike ARDUINO for version

Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

the difference I'm getting at is that ARDUINO covers all arduino variants but I'm assuming Spark core is not an arduino variant (and included in the Arduino distribution) unless I'm mistaken.

Copy link
Contributor Author

@jacobrosenthal jacobrosenthal Aug 1, 2014

Choose a reason for hiding this comment

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

No Spark isnt an arduino variant .. I have no idea what it takes to get that blessing..

Copy link
Member

@soundanalogous soundanalogous Aug 1, 2014

Choose a reason for hiding this comment

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

As long as it's in line with an arduino variant (having the correct pin defines, etc) it should be okay to include. Up to this point everything in firmata/arduino has been specific to boards included in the core distribution (well at least included at one time or another... some have been dropped).

Are all of the same basic features as something like an Arduino UNO supported for Spark Core (I2C, analog pins can be set as digital pins, etc) or are exceptions needed - not sure how to handle that.

Copy link
Member

@soundanalogous soundanalogous Aug 3, 2014

Choose a reason for hiding this comment

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

Reviewing this again I've realized that HardwareSerial should no longer be included in Firmata.cpp anyway. Stream.h should be included in it's place and then HardwareSerial.h should be included in StandardFirmata.ino before the Firmata.h include. If someone creates an ethernet version of StandardFirmata they would include Ethernet rather than HardwareSerial. So someone should try to get Stream included (and properly integrated) in spark/core-firmware. I'll make the necessary updates to Firmata.

Copy link
Member

@soundanalogous soundanalogous Aug 3, 2014

Choose a reason for hiding this comment

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

Here's the proposed change to Firmata.cpp (and all Firmata examples): #144.

Copy link

@blakejakopovic blakejakopovic Aug 4, 2014

Choose a reason for hiding this comment

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

For reference, here is an ethernet version of StandardFirmata https://github.com/peterschwarz/standard-firmata-ethernet, and a WIP for wifi (using cc3000 chip) https://github.com/peterschwarz/standard-firmata-cc3000

Copy link
Member

@soundanalogous soundanalogous Aug 4, 2014

Choose a reason for hiding this comment

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

@blakejakopovic We have Ethernet support in the configurable branch. I recommend adding a similar keep alive functionality (see the maintain method here) to your library.

#include "HardwareSerial.h"
#endif

extern "C" {
#include <string.h>
Expand All @@ -28,8 +31,8 @@ extern "C" {

void FirmataClass::sendValueAsTwo7bitBytes(int value)
{
FirmataSerial->write(value & B01111111); // LSB
FirmataSerial->write(value >> 7 & B01111111); // MSB
FirmataSerial->write(value & 127); // LSB
FirmataSerial->write(value >> 7 & 127); // MSB
}

void FirmataClass::startSysex(void)
Expand Down
2 changes: 1 addition & 1 deletion readme.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
#Firmata
#Firmata has been acccepted as an official fork. Please see https://www.github.com/firmata/spark

Firmata is a protocol for communicating with microcontrollers from software on a host computer. The [protocol](http://firmata.org/wiki/Protocol) can be implemented in firmware on any microcontroller architecture as well as software on any host computer software package. The arduino repository described here is a Firmata library for Arduino and Arduino-compatible devices. See the [firmata wiki](http://firmata.org/wiki/Main_Page) for additional informataion. If you would like to contribute to Firmata, please see the [Contributing](#contributing) section below.

Expand Down