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
Pilot Event (PEV) start #647
Conversation
AddFloat(_("PEV start wait time"), | ||
_("Wait time in minutes after Pilot Event and before start gate opens. " | ||
"0 means start opens immediately."), | ||
_T("%.0f min"), _T("%.0f min"), 0, 30, 1, false, UnitGroup::ALTITUDE, |
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.
UnitGroup::ALTITUDE
sounds wrong
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 agree. I forgot I did this hack.
I hesitated to add another UnitGroup for MINUTES
just for this use case though. Would you recommend introducing UnitGroup::MINUTE
or rather use UnitGroup::NONE
here? I don't remember exactly what was wrong now, but UnitGroup::NONE
didn't work here out of the box. I can collect more details on this if that's helpful.
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.
No new UnitGroup. XCSoar's units are for values where the user may choose different display units, but the code internally uses the base SI unit.
The first thing that's somewhat "wrong" with your code is that it doesn't use SI units; the SI unit for duration is seconds, not minutes. I'm not 100% sure if I want to demand you to switch to SI units - there may be good reasons to stay with minutes, but using SI units is never wrong, because it avoids a whole class of bugs.
The second thing that's wrong - if you're not going to use a UnitGroup, don't use the AddFloat overload that takes a UnitGroup parameter. Don't use UnitGroup::NONE.
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 have discovered AddTime()
, which works with seconds. This was a good enough incentive to switch from minutes to seconds.
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 think you may need to adjust the help text. "in minutes" is redundant because the UI already displays the unit.
src/Input/InputEventsActions.cpp
Outdated
const RoughTimeSpan ts = RoughTimeSpan(new_start, new_end); | ||
start.open_time_span = ts; | ||
|
||
protected_task_manager->SetOrderedTaskSettings(ots); |
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.
This is a very broad call, when all you need is a way to change the task's open_time_span
attribute. To avoid developers to do stupid/dangerous things with this method, let's make a dedicated method for just setting open_time_span
.
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.
Changed that to SetStartTimeSpan
instead.
Pilot Event (PEV) start procedure is defined by two parameters: "wait time" and "start window". These parameters define when start opens relative to "pilot event" and for how long it stays open. These parameters are properties of a task and are configurable by the user. Defaults for a new task are configurable in System Config -> Task Defaults -> Task Rules.
Store PEV start settings in a .tsk file.
Logger devices can send a special device-specific NMEA message to the device to simulate the press of the physical "Event" button. This is useful in case the logger is not reachable by the pilot directly.
The PEV button is a available in menu: Nav -> Nav -> Pilot Event.
Unfortunately, new Pilot Event texts can not be translated. Can you please have a look on this issue? |
@MikeBravo15 from what I can tell, strings are marked as translatable (wrapped in |
Not a big expert on the code, it seems to be OK, but text strings do not appear in Weblate for translation (none of them) |
I see, perhaps the translation template needs to be updated (somehow). Will try to figure it out. |
The POT file has not been regenerated yet. |
@MaxKellermann will it haappen automatically or someone should do something to regenerate it? |
Manually. |
Brief summary of the changes
This adds support for Pilot Event start procedure, recently introduced for use in gliding competitions. See rules for world and continental gliding championships, page 28, 29 for more details.
This PR introduces a new button, available under Nav -> Nav -> Pilot Event. When pressed, a time window for a valid start is configured according to two new user-configurable settings for the task. This feature takes advantage of existing "start open time" and "start close time" parameters as well as existing "Start open" and "Start reach" infoboxes to aid the pilot.
Expected mode of operation: when "Pilot Event" start is used in a competition day, pilot will set "PEV start wait time" and "PEV start window" task properties according to values, announced during the briefing. Later, in the air, in order to make a valid start, the pilot will press the "Pilot Event" button in XCSoar (same way they would press the button on the physical logger hardware), wait the specified amount of time and then cross the start line while start window is open. Once "Pilot Event" is pressed, "Start Open" infobox will start the countdown to the new start open time.
The pilot event is announced on connected devices (by the driver-specific NMEA message) - this is useful in case the logger device is physically unreachable by the pilot (e.g. behind the instrument panel).
Related issues and discussions
This PR is ready for review, but not ready to merge yet. I'd like to make a test flight in order to confirm the integration with FLARM is working as expected.
Closes #646.