Skip to content
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

[settings] add optional target attribute to settings #1157

Merged
merged 10 commits into from
Jul 30, 2015
Merged

Conversation

flixr
Copy link
Member

@flixr flixr commented Apr 2, 2015

If not target attribute is given, the settings are added for all targets like before.

This makes it easier to add settings files to the conf that are only valid for some targets like nps or some that you don't want when using the test progs.

Beware of: if you compile a target that unloads some settings or modules (e.g. the sim) after you uploaded to the ap, the var/aircrafts/<ac>/settings.xml file that has all usable settings combined will change ~~~and the hence the wrong ones used in GCS and settings prog~~~.

Now the calculation of the MD5sum is taking unloaded settings into account, meaning it will change and warn the user if the last compiled target has different settings.

@@ -1,6 +1,6 @@
<!DOCTYPE settings SYSTEM "../settings.dtd">

<settings>
<settings target="ap|nps">
Copy link
Member Author

Choose a reason for hiding this comment

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

why restrict this to ap and nps?
That means you can't use it with demo and test targets...

Choose a reason for hiding this comment

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

Because it fails with sim target where imu.c is not compiled. If you want to keep it without a specific target, I suggest to fix sim then.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, how about something like flixr@493fde0 to add the possibility to exclude a list of targets with target="!foo|bar"

Choose a reason for hiding this comment

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

The try is incomplete, String.get may return Invalid_argument

@gautierhattenberger
Copy link
Member

do we merge this one ?

@flixr
Copy link
Member Author

flixr commented Apr 26, 2015

It is ok to merge for me.
But we definitely need to better handle having a different settings.xml file after compiling another target.
At the very least something like a check for the number of settings?

@flixr
Copy link
Member Author

flixr commented Jul 25, 2015

Should we merge this befoer v5.6 regardless of the missing checks if you had unloaded settings/modules?

@gautierhattenberger
Copy link
Member

It is very useful but sadly it is not possible to make just one line insensitive in the settings selection box. At least we could add a warning when unloading settings ?

flixr and others added 5 commits July 25, 2015 16:32
I wanted to make the line unsensitive, but it doesn't seems to be
possible. Either all the lines or nothing. As a fallback, the settings
is unselected. It is not perfect (some stuff might be missed, you can
check it back, ...) and the settings are removed at generation time
anyway.
Not sure it can be done better unless someone finds the way to control
the list element sensitivity one by one.
@flixr
Copy link
Member Author

flixr commented Jul 25, 2015

I'm not so much concerned about the selection box, that seems to "only" be an inconvenience to me.
As said before I don't like the increased probability of mismatched settings with potentially even catastrophic outcome if you unknowingly change the wrong one.

What about adding another MD5sum for only the settings and sending that with ALIVE?
The we could check this when loading settings or the GCS to see if they match...

@flixr
Copy link
Member Author

flixr commented Jul 25, 2015

Or better, also do the filtering for used settings (unloaded modules with settings, targets in settings) when creating var/aircrafts/foo/conf/conf_aircraft.xml of which the md5sum is computed.

I think that would be the best solution, or am I missing something obvious here?

@gautierhattenberger
Copy link
Member

It seems that we are currently only removing the settings that are not selected in the pprz center. I guess it should also work if we correctly unload them when the module is not valid for the target.

@flixr
Copy link
Member Author

flixr commented Jul 27, 2015

Yes and the settings that are directly added but don't match the target, could you do that in this branch?

this should prevent mismatch when settings are unloaded due to
unsupported targets
@flixr
Copy link
Member Author

flixr commented Jul 27, 2015

The filtering now only works correctly from within paparazi center if you don't enable unloaded setting again.
As you said this is is partially a problem with the the list element.
IMHO the filtering should be done properly in gen_aircraft.ml so that the generated conf_aircraft.xml will be always correct and it also works if called directly from the command line....

@gautierhattenberger
Copy link
Member

I don't get it. If I force the selection of an unwanted settings file (like nps.xml for ap target), it will be removed from conf_aircraft.xml and will not appear in the generated settings as well.
So conf_aircraft.xml really hold the conf used for the last build.

@flixr
Copy link
Member Author

flixr commented Jul 29, 2015

Ok, nevermind my previous comment.... that indeed works as expected now.

However I think it would be better to not "uncheck" the setttings file in the paparazzi center if it is unloaded.
Otherwise you suddenly have the settings file disabled if you change to a target that supports it again...

@flixr
Copy link
Member Author

flixr commented Jul 30, 2015

Actually I think the unloaded should not change the conf_aircraft.xml (and hence the md5sum) unless they have settings.
If they don't have settings, there is not reason to have a different md5sum as the settings.xml will be the same.

@flixr
Copy link
Member Author

flixr commented Jul 30, 2015

Nevermind, seems to work as expected.

flixr added a commit that referenced this pull request Jul 30, 2015
[settings] add optional target attribute to settings

f not target attribute is given, the settings are added for all targets like before.

This makes it easier to add settings files to the conf that are only valid for some targets like nps or some that you don't want when using the test progs.

Beware of: if you compile a target that unloads some settings or modules (e.g. the sim) after you uploaded to the ap, the var/aircrafts/<ac>/settings.xml file that has all usable settings combined will change.
Now the calculation of the MD5sum is taking unloaded settings into account, meaning it will change and warn the user if the last compiled target has different settings.
@flixr flixr merged commit ade0ad6 into master Jul 30, 2015
@flixr flixr deleted the settings_target branch July 30, 2015 11:05
@flixr flixr added this to the v5.6 milestone Jul 30, 2015
@FlightEnabled-Admin
Copy link

Hi, I'm just getting back into paparazzi. I tried to follow the mac
paparazzi install instructions but they appear to not work, is there an
up-to-date mac install setup I can follow?

On Thu, Jul 30, 2015 at 5:05 AM, Felix Ruess notifications@github.com
wrote:

Merged #1157 #1157.


Reply to this email directly or view it on GitHub
#1157 (comment).

@flixr
Copy link
Member Author

flixr commented Aug 2, 2015

@flightenabledOrg please use the mailing list for such questions and not some totally unrelated issue...
Also please try to include as much details as possible (OS version, paparazzi version, errors you got, etc). "does not appear to work" is to unspecific to give any real help

@FlightEnabled-Admin
Copy link

Whoops, I apologize.

@flightenabledOrg https://github.com/flightenabledOrg please use the
mailing list for such questions and not some totally unrelated issue...
Also please try to include as much details as possible (OS version,
paparazzi version, errors you got, etc). "does not appear to work" is to
unspecific to give any real help


Reply to this email directly or view it on GitHub
#1157 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants