kevinabrandon / AboveTustin

ADS-B Twitter Bot. Uses dump1090-mutability to track airplanes and then tweets whenever an airplane flies overhead.
MIT License
72 stars 21 forks source link

FlightAware flight info API call #15

Closed spaprzycki closed 6 years ago

spaprzycki commented 6 years ago

Hi Kevin,

I have started using your awesome AboveTustin last night - thanks for coding it!

I though pulling some more info on the flights passing by would be nice, so here is my first commit that uses FlightAware API calls [*]. For now I am only exposing origin and destination airports to the tweet vars, but some more data can be pulled - ie. airplane data, weather info etc. I will be hacking away in my free time, but I though this will be most useful for the start.

[*] https://flightaware.com/commercial/flightxml/pricing_class.rvt

In my opinion FA API is useful as most people running the ADS-B setup would most likely be FA feeders anyway, so the free API limits are actually very usable!

Any feedback (good or bad) will be most welcome.

Regards, Sergiusz

kevinabrandon commented 6 years ago

Very cool! I've wanted this functionality since I created the project but have been too busy to add it myself.

So looking at the code I have a couple questions.

  1. What happens if the user doesn't have a FA API key? It looks like it attempts to access it on each tweet? Maybe you could add a configuration option to completely disable it for those users?
  2. What happens if FA can't find the source or destination airports, or if their service is down? What would the tweet look like? this maybe: "Flight AAL1208: > 0.2mi away @ 1500ft..." Maybe we need two templates one for when we have FA data and one when we don't?

Anyway, I'm super excited to add this. Thanks a ton.

spaprzycki commented 6 years ago

Hi Kevin,

Thanks for kind words :)

I feel quite stupid to be honest - I should have though about these two points!

As for 1 - this is easily done and I will implement this tomorrow into my branch.

2 - your idea about 2 templates would work, then within the script we can chose one or the other depending on the info we managed to gather - I will implement that tomorrow as well.

In the long run probably cleanest would be to offload building of the tweet message content to templating engine. Employing Jinja2 may sound like a bit overkill, but this is the only one I have some small experience with.

Btw. Obviously I'm not professional coder (which you know by now) and on top of that Python wouldn't be traditionally my language of choice (I'm far more comfortable in PHP), but I'm keen to keep practicing Python - hence my contribution :)

Regards, Sergiusz

kevinabrandon commented 6 years ago

Sergiusz,

As far as "professional" coding, please be aware that this whole project is mostly a hack :) . My experience is in C based languages (C/C++/C# and more recently golang). This project is my first and only real python coding, and it was mostly hacked together in a few days as a personal challenge. So I do not want to vouch for the quality of the code.

As far as templating engines, I don't have any experience with them and so I used the simplest thing I could find (string.Template). But it would be really nice to have some conditional stuff. For example I have it add different hashtags if an airplane is flying in given altitude ranges. Currently that logic is hardcoded, and the individual users can't change those values using their own templates in the configuration file.

So if you feel the urge, feel free to investigate templating engines. But for now I think 2 templates one for when FA data is available and a default one to fall back on is perfectly sufficient for this pull request.

Thanks again! Kevin

spaprzycki commented 6 years ago

Hi Kevin,

OK, so I have made the changes as discussed. Had lots more problems then expected, but hopefully finally this should work. Should - I mean it does work for my test setup for all 3 scenarios:

  1. FA disabled
  2. FA enabled, but not enough data received
  3. FA enabled and enough data received

I still don't like the code I wrote, so I will continue working on it, hopefully will deliver some quality code over next few days in another PR. Totally agree that taking custom hashtags into conditional template would be nice, also it would greatly help to tidy up code and to utilize other data from FA (like airport full names), which would for now required some small changes to the script itself.

Also I was thinking that some logging to file would be nice with 'verbose' flag, so this can be used when troubleshooting and without flag for normal operation. I would be happy to implement that as well. What you think?

Regards, Sergiusz

kevinabrandon commented 6 years ago

Looks great... I'm going to try and get my fa api key setup for my own feed. Also, what's your feeder's twitter handle? I'd like to follow it.

As far as what to do next, feel free to work on anything that interests you. Logging is something that can definitely be improved. There are a few other outstanding issues as well if you are looking for other ideas for things to work on.

Anyway thanks again for the great contribution

kevinabrandon commented 6 years ago

Just a heads up @spaprzycki that I made a couple changes.

  1. It was crashing every time there was a tweet for me. It was because the output variable hadn't been initialized before you started setting it. So I added a output = dict() near the top of your FlightInfo() function.
  2. I wasn't getting any tweets with airports in them, so I looked at the return json and saw that the status showing the current flight was "En" and not "On". Maybe sometimes it needs both?

Overall the FlightInfo() function is a bit confusing to me because it can return different things. I'm not a python programmer, so I don't know what is idoimatic, but for me, coming from C based languages, it is super weird to be able to return different types of objects from a function.

If I were to do it, I'd probably write a function like this: def FlightInfo(ident, templateArgs, username, api_key) It would always return either True on success, or False on failure. templateArgs would be an already defined dictionary that you add items to on success. Finally you'd call FlightInfo() from inside the Tweet function, and give it the same templateArgs.

What do you think?

spaprzycki commented 6 years ago

Hi Kevin,

As for 1 - yes, that's clearly my oversight... the weird thing is that is hasn't been crashing for me! I have actually left the script to itself for a while and I have just noticed there was no tweet or log output since around noon yesterday, so it may be that crash... I have changed logging now, so when script is restarted it doesn't override the previous log, but append instead. I'll let it run for a while to see if this is the same crash.

2 is interesting... I wish FA have documented their API little bit better. I have checked few flights and they were all status = On, I guess my sample wasn't large enough.. Also you're in US, I'm in Europe, so there may be some differences in data... you Americans like to be different from us Europeans (or the other way around) ;-)

Additionally I have asked on FA forum if they plan to make filtering work on the API call we use here as it is huge waste to pull lots of flights data, loop through the results and discard all but 1. Filter for "on" flights sounds to me like very logical choice, but they don't see it that way I think :)

Last by not least - all I have in my defense is that I have been coding PHP for far too long, where things are a lot more relaxed... more relaxed then in Python and A LOT more relaxed then in C. I know it doesn't really defend me and I take this as good lesson, so thank you! I will try to re-work the function when I get a chance. Btw. Same "defense" applies to comment 1 - PHP initialized variables in-flight for you.

Ah, did I mentioned that I'm not coding for life...? I would probably starve by now if I was... :D Telecoms is easier! ;-)

Regards, Sergiusz

spaprzycki commented 6 years ago

Also, what's your feeder's twitter handle? I'd like to follow it.

https://twitter.com/Sky_Ntmk