search

voodootikigod / voodoospark

A RPC based firmware for a Spark Core device (like being connected, but without the wire!)
MIT License
145 stars 35 forks source link

fix for compilation error against firmware v 0.4.7 #55

Closed Gabriel-Lacatus closed 8 years ago

Gabriel-Lacatus commented 9 years ago

Fixes #54

edsadr commented 9 years ago

:+1: please merge

BrianGenisio commented 9 years ago

I ran into this tonight. Before this gets merged, I have two questions about this:

  1. First, this problem exists because the 0.4.7 firmware introduces a pulseIn function. By keeping our name the same as the system pulseIn, I fear that we don't know for certain that our function will be called. Instead of casting and hoping it disambiguates properly, how about changing the function to pingPulseIn or something... that way we are disambiguating by name, not by casted parameters?
  2. Along the same lines, do we even need our own pulseIn any longer? Can we just delete our implementation of pulseIn and use the system version, if it exists? It might require a #define so it works with old firmwares.
Gabriel-Lacatus commented 9 years ago

The naming collision is a bit unfortunate. From looking at the code of both functions it looks like they are doing fairly similar things but I notice at least two differences that have external effect:

  1. The system function times out after 3 seconds whereas the voodoospark one times out in 10 seconds
  2. The system function waits until the pin reaches the opposite state before starting to count whereas the voodoospark function starts counting directly if the pin is already in the desired state at the time of the function call

I guess the timeout discrepancy is not terribly problematic but the second difference might cause slightly different outcomes in some cases?

BrianGenisio commented 9 years ago

Knowing this, I'd prefer to just change the name of pulseIn to pingPulseIn to avoid the name collision completely, but maintain compatibility with the ping command. It would be awesome if we can get this merged soon. We have to flash 110 chips pretty soon here... it would help if we can use an official drop of the firmware.

@rwaldron Do you agree with this?

rwaldron commented 8 years ago

@BrianGenisio yeah, pingPulseIn is legit. I'll land this and then change the name entirely. Thanks everyone.

rwaldron commented 8 years ago

https://github.com/spark/particle-cli/pull/169