-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Store the git SHA of test-flown aircraft #1921
Conversation
1ce339e
to
f04b0f6
Compare
sw/supervision/pc_aircraft.ml
Outdated
@@ -104,6 +105,25 @@ let gcs_or_edit = fun file -> | |||
| 2 -> ignore (Sys.command (sprintf "%s -edit '%s'&" gcs file)) | |||
| _ -> failwith "Internal error: gcs_or_edit" | |||
|
|||
let checkout_git_version = fun sha -> | |||
(Sys.command (sprintf "git checkout '%s'" sha)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really found of doing this since it may cause a big mess if you have local changes, and if the checkout works, it will most likely require a make
at top level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes indeed. Moreover you will then be in detached head mode, which might freak out some people. And a make will be needed so you need to go to the prompt anyway. A copy SHA could be better, but that didn't work out in OCAML. Any tips?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The checkout option was removed in favor of a GITK button, which also allows to check out the revision but allows much more.
Please also keep in mind that this has to be something optional and has to fail gracefully if you don't have a git checkout (e.g. used the tarball). |
Can this be made optional? For example an optional tag in the airfame config file? Seems unnecessary to show it for all users in the pprzcenter (I assume that is what the glade edit is for). At least in our workflow we never work in the detached state - instead we have "flight_tested" branches. It probably doesn't help with this problem, but at least we would like to keep our workflow if possible. |
|
@dewagter OK, sounds good to me. |
@dewagter the information you want to keep doesn't have to be in conf after all. What about adding a button to save at the end of a flight relevant information: conf of the airframe, git SHA, a dialog box to enter extra relevant information, log file name, the file in which data are saved (might be a DB in the future). This way we would have a kind of flight log book, that you can also share. |
@podhrmic @gautierhattenberger Thanks for the feedback and tips.
But to me, this is DIFFERENT from the current pull request.
SO:
In other words: this is about TESTING (all targets of an airframe, and all its modes), while the idea of gautier is more about logging, and other discussions are about keeping track of who flies on which branch. For me 3 very different things that can complement each other and live in parallel. IMHO the most pressing one for me is the TESTING and SHARING/STORING test results. |
@dewagter In some cases Hardware in the loop - HITL (#1831) can be useful for testing, but it is limited to using an external INS (Vectornav, Xsens). I am curious what kind of errors you typically encounter when trying to fly (i.e. what errors are we trying to prevent by testing)?
I am trying to break down the issues because it is not always practical to test fly (time, resources, place). For example, we had lot of problems with 3) so we had to upgrade HITL to catch them and cut down the cost of testing. If you are encountering lot of problems of a specific type, maybe it is worth developing some testing tool to prevent them? Just a thought. Otherwise as I mentioned, having an optional SHA tag is fine for me, but for practical reasons I don't think we'll be using it. |
Most often the problems are of type 3) and 4). E.g. optic flow stabilization is developed and tested. Then a pull request But also: indi is upgraded, and test flown on bebop. A pull request is made My point being: I know not everyone will go out to test every development Rephrased: "SHARE your test results." Any alternative idea is welcome to Just out of interest I wonder when you say: "I don't think we'll be using -Christophe On Fri, Nov 4, 2016 at 6:18 PM, Michal Podhradsky notifications@github.com
|
it is mostly a logistical issue - we use a paparazzi fork, and although I am slowly merging our code with master, it is not ready yet (so airframes and certain custom modules not in master). When this happens it will be more relevant, although at this point I think we would use the latest stable branch, and update periodically to the new stable release. Also, we have different people developing the code (mostly me), and different people flying (pilots, operators). Operators aren't necessarily familiar with the code itself nor github, so the simpler for them, the better. Plus we are benefiting from much simpler code (using only a single MCU and no fun stuff like optical flow etc:-/ ), so we can fully test with HITL. |
@dewagter for this I don't think doing git checkout or diff is nice. But does it help if at least the SHA is stored ? Also it means that when using a tarball release, the function should properly fail and maybe just return the current version. |
@gautierhattenberger: we can do diff (of only the files used by the Yes, only storing the sha (and preferably a comment/note) is OK. I was thinking of an analyse button in the select-conf.py program that -Christophe On Fri, Nov 18, 2016 at 10:40 PM, Gautier Hattenberger <
|
can you update the PR to only keep SHA/comment and fix conflict then ? |
@dewagter can you resolve the conflicts here? |
@dewagter any updates here? @gautierhattenberger @flixr should we keep this open? |
d53b23c
to
fb7b9fc
Compare
All nice! Note that although airframe age computation or last testflown should should more indicative. since we have airframe years old, still fly well and even on a regular basis we should not be forced to push results else an airframe gets removed. maybe add a flag would be better like someting as "not testflown lately" This PR improvement will not hinder others so a merge would be great. |
If you add an airframe XML in master (which is not obviously called test/example), then most people will expect it existed one day. If it existed, it probably flew one day. But it also might not fly well anymore in recent versions. (Especially vision based controlled stuff has this a lot) My point: not working airframes are bad for the project. If you simply tell which version worked for you, you provide extremely valuable information: 1 anyone can check it out quickly Problem:
About "forced to push results" There is no point in having an airframe in master which does not exist anymore, other than examples. If it still exists, but you prefer older code, then maybe we also want to remove the XML from master and link to the older version. And if you want to keep it in master, maybe you want to check if recent code also works for you. |
7888684
to
6ae0b63
Compare
081bdb1
to
d19c26f
Compare
d19c26f
to
5d9acc5
Compare
5d9acc5
to
a2c65a4
Compare
What else should I change to accept this pull request:
|
change:
personal use:
users:
project: