Closed yocontra closed 12 years ago
I did the same with my code last time, but as Felix pointed out, what would happen if i:
client = require('ar-drone').createClient();
client.takeoff()
And the battery is low? Also navdata.demo is not sent by default, and enabling it to do so is not a good idea as per Felix comments.
While custom callbacks events might be cool, this one does not solves the battery error issue. But aside that i think this is similar to: #13, so since it has test it might be a good addition! (Maybe add tests to that PR and merge it with this?)
client = require('ar-drone').createClient();
client.once('navdata', function(){
if (client._lastBatteryl < 50){
process.exit();
}
client.takeoff()
});
Couldn't you use the first navdata event almost as a client.ready? I've implemented any calls to drone data as async functions in my util lib that wait for the next navdata event - is it unreasonable to do that?
No, since the navdata fires async, the drone will try to takeoff anyways (regardless when you bind it). What the #6 issue asks for is to throw an error when the battery is low and the takeoff() method is called, while avoiding the use of .emit('navdata') and such. That's why is a tricky one.
Why not just check the battery level before trying to take off though in your application code? AFAIK the drone won't launch if it's under 20% battery anyways right?
That's correct, but since you can still call .takeoff() we wanted to provide a way to say "hey, the battery is low, charge it".
And even if you checked the battery state before, you'd have to use promises or add all your app logic inside a callback for navdata or something like that, we just want a simple error. It's unrelated to this PR though.
@brunolazzaro IMO the battery thing has different concerns / problems than this patch.
@Contra Did you test that the fly states map to the events you've selected for them (not by a unit test, but by flying)? If so, this patch LGTM, but I'd like this stuff to be extracted out into it's own little private function. Can you do that?
@felixge - Yeah this has all been tested via flying my drone around the office. If this looks good there are a lot of other useful events that could be added from droneState and demo navdata messages. I'll split this out into a new fn now
Ok, ping me once you did the split and I'll merge.
@felixge - done
Merged this. Thanks! (I'm not a huge fan of the internal implementation, I hate inlined functions, but whatever, the overall functionality is good)
Oh, this still needs some docs so!
This will make a drone client emit landing, landed, takeoff, flying, batteryChange, altitudeChange, and lowBattery events based on incoming navdata.
All of the tests pass but I'm not familiar with the testing framework so you might want to double check before pulling this in
Callbacks for .takeoff() and .land() could be implemented as easy as
this.once('landed', cb)