Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[Accepted with Revisions] SDL 0170 - SDL behavior in case of LOW_VOLTAGE event #489

Closed
theresalech opened this issue May 9, 2018 · 18 comments

Comments

@theresalech
Copy link
Contributor

theresalech commented May 9, 2018

Hello SDL community,

The review of "SDL 0170 - SDL behavior in case of LOW_VOLTAGE event" begins now and runs through May 15, 2018. The proposal is available here:

https://github.com/smartdevicelink/sdl_evolution/blob/master/proposals/0170-sdl-behavior-in-case-of-Low-Voltage.md

Reviews are an important part of the SDL evolution process. All reviews should be sent to the associated Github issue at:

#489

What goes into a review?

The goal of the review process is to improve the proposal under review through constructive criticism and, eventually, determine the direction of SDL. When writing your review, here are some questions you might want to answer in your review:

  • Is the problem being addressed significant enough to warrant a change to SDL?
  • Does this proposal fit well with the feel and direction of SDL?
  • If you have used competitors with a similar feature, how do you feel that this proposal compares to those?
  • How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
    Please state explicitly whether you believe that the proposal should be accepted into SDL.

More information about the SDL evolution process is available at

https://github.com/smartdevicelink/sdl_evolution/blob/master/process.md

Thank you,
Theresa Lech

Program Manager - Livio
theresa@livio.io

@jacobkeeler
Copy link
Contributor

jacobkeeler commented May 11, 2018

Is the problem being addressed significant enough to warrant a change to SDL?

I would say yes, but I'm not familiar with how other OEMs normally handle such low voltage scenarios, so I'm not certain how relevant this solution is to anyone besides Ford.

Does this proposal fit well with the feel and direction of SDL?

As I've mentioned, this solution might be a bit too specific to one OEM, but I'll let others weigh in on this matter. The addition of a new channel for communication between Core and the HMI seems to not fit as well with the direction of SDL, because this would be a lot to manage. On the other hand, I'm not certain if a better solution to manage these high-priority messages exists using the current communication channels.

How much effort did you put into your review? A glance, a quick reading, or an in-depth study?
Please state explicitly whether you believe that the proposal should be accepted into SDL.

A quick reading. If after others weigh in on this proposal it appears that OEMs besides Ford require this sort of functionality, then I believe that this proposal should be accepted into SDL.

@Jack-Byrne
Copy link
Contributor

In the HMI API there exists an OnAwakeSDL RPC.

    <function name="OnAwakeSDL" messagetype="notification">
        <description>
            Sender: HMI->SDL. Must be sent to return SDL to normal operation after 'Suspend' or 'LowVoltage' events
        </description>
    </function>

Also there is a SUSPEND enum which is used in OnExitAllApplications.

<enum name="ApplicationsCloseReason">
  <description>Describes the reasons for exiting all of applications.</description>
  <element name="IGNITION_OFF" />
  <element name="MASTER_RESET" />
  <element name="FACTORY_DEFAULTS" />
  <element name="SUSPEND" />
</enum>

Instead of creating a new communications channel, can the problem described by the proposal be solved by using "SUSPEND" or adding "LOW_VOLTAGE" to the ApplicationsCloseReason enums as well as using the OnAwakeSDL RPC?

@robinmk
Copy link
Contributor

robinmk commented May 14, 2018

The aim is to suspend SDL when a Low voltage condition occurs. A potential problem with using the existing messages in HMI API is that these messages can be queued behind other HMI API messages and the system will likely suspend SDL before Core gets an opportunity to de-queue and process the messages. Hence the request for a separate communication mechanism.
However, I would like to get feedback from others as well on the likelihood of this problem on their platforms.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented May 14, 2018

Also I believe the proposal is suggesting we use "mqueue" as method to deliver low voltage events to core?

I thought that "mqueue" was the method of communication from Core to HMI specific to the Sync 3 implementation (in place of websockets). This type of communication between core and the HMI does not exist at all in the open source project. The only form of communications between core and the HMI is through web sockets or dbus.

If this proposal means that mqueue must also be implemented in the project then I feel like there are a lot of details missing in the proposal.

Maybe I am misunderstanding something but the proposal is written as if "mqueue" already exists in the project.

@robinmk
Copy link
Contributor

robinmk commented May 15, 2018

The idea is to agree upon a form of communication which can work for all. Message queues was suggested as an example.

@LuxoftAKutsan
Copy link
Contributor

@jacobkeeler :

I would say yes, but I'm not familiar with how other OEMs normally handle such low voltage scenarios, so I'm not certain how relevant this solution is to anyone besides Ford.

SDL should propose how to handle Lov Votlage event shouldn't it?

The addition of a new channel for communication between Core and the HMI seems to not fit as well with the direction of SDL, because this would be a lot to manage.

That is the drawback of this approach, but for handling low voltage sdl should provide low consuming energy way of minimal communication.

@JackLivio

In the HMI API there exists an OnAwakeSDL RPC.
Description of OnAwakeSDL should be fixed. It should Awake only after suspend but not low voltage.

Instead of creating a new communications channel, can the problem described by the proposal be solved by using "SUSPEND" or adding "LOW_VOLTAGE" to the ApplicationsCloseReason enums as well as using the OnAwakeSDL RPC?

SUSPEND is not the same as LOW_VOLTAGE. SUSPEND is regular state for example when you exited the car, but HU is still working for some time to startup faster in case if you will return back. Low Voltage is not regular state, it is exceptional state. In case of low voltage we should consume as few resources as possible. That's why we should shutdown all transports and event reject any communication with Applink via MessageBrocker. This proposal is for creating low consuming transport channel, without any websockets, Json parsing and notifications subscriptions managment, and heavy adapters. It should just connect to mqueue and be ready to react on minimal string received from Applink side.

Maybe I am misunderstanding something but the proposal is written as if "mqueue" already exists in the project.

mqueue does not exist in project, but there should not be created additional mqueue transport adapter. Mqueue should be separate channel that would be as simple as it possible to do, and Manage sending Low Voltage/Awake signals to other components of SDL.

@yang1070
Copy link
Contributor

In stead of a mqueue, I would like to see an additional emergency interface (or any other proper name) that are parallel to the existing ui, tts, VehicleInfo, navigation interfaces. And that can be implemented either by sockets, dbus or mqueue.

@LuxoftAKutsan
Copy link
Contributor

In case of additional interface we will need to keep a lot of threads active on select ( whole him communication) , and processing him requests. This activity is rather expensive for Low voltage state

@Toyota-Sbetts
Copy link
Contributor

"SUSPEND is not the same as LOW_VOLTAGE. SUSPEND is regular state for example when you exited the car, but HU is still working for some time to startup faster in case if you will return back. Low Voltage is not regular state, it is exceptional state. In case of low voltage we should consume as few resources as possible. That's why we should shutdown all transports and event reject any communication with Applink via MessageBrocker. This proposal is for creating low consuming transport channel, without any websockets, Json parsing and notifications subscriptions managment, and heavy adapters. It should just connect to mqueue and be ready to react on minimal string received from Applink side."

While I'm not sure we can handle low voltage this way, I do see the advantage of this type of emergency suspension of SDL Core. Since SUSPEND is already used, maybe we could use HIBERNATE or ESTOP/ESUSPEND? I think this type of function could be called in cases other than low voltage.

Regarding the additional communication channel, as the proposal states I'm also concerned to make any communication channel a requirement to OEMs. I don't think the communication method has to be tied to this functionality so I agree with the direction @yang1070 mentioned that we should seek to have this more generic or have the communication method to be defined by the OEM.

@Jack-Byrne
Copy link
Contributor

Jack-Byrne commented May 15, 2018

Core has a signal handler in life_cycle.cc https://github.com/smartdevicelink/sdl_core/blob/master/src/appMain/life_cycle.cc#L213

Could we define behavior for the Linux signals SIGTSTP and SIGCONT as a way to stop/resume core in the event of low voltage?

There is also a signal "SIGPWR" which is used for Power failure but I'm not sure if that is applicable here.

@Jack-Byrne
Copy link
Contributor

After a quick test, this also works in stopping and resuming core

Start Core. Find the PID of Core (mine was 33230).

This will stop core.

$ kill -STOP 33230 

This will resume core to the same place before the stop signal.

$ kill -CONT 33230

@theresalech
Copy link
Contributor Author

The Steering Committee voted to defer this proposal, keeping it in review until our meeting on 2018-05-22, to allow time for SDLC Members to investigate the option of using signals to stop/resume core in the event of low voltage, as described by JackLivio. Delivering a signal could be a quicker and more power-efficient way than delivering a new form of communication, such as mqueue. It was noted that preliminary signal testing was conducted using Ubuntu 14, and signal behavior should be tested for OEM-specific implementations to ensure the solution is viable across platforms. Lastly, the project maintainer advised that we could define behavior on signals within the signal handler that currently exists within Core.

@robinmk
Copy link
Contributor

robinmk commented May 22, 2018

I like Jack's idea of using user defined signals. Our preliminary analysis indicates that this is possible. We are doing some more analysis to check if the same mechanism can be expanded to other signals/messages as well.

@theresalech
Copy link
Contributor Author

The Steering Committee voted to defer this proposal, keeping it in review until our meeting on 2018-05-29, to allow more time for SDLC Members to conduct additional testing and analysis of the alternative option described by JackLivio: using signals to stop/resume core in the event of low voltage.

@robinmk
Copy link
Contributor

robinmk commented May 25, 2018

After analysis, we would like to propose the use of 3 signals of the range SIGRTMIN - SIGRTMAX for LowVoltage, WakeUp and IgnitionOff.

@theresalech theresalech changed the title [In Review] SDL 0170 - SDL behavior in case of LOW_VOLTAGE event [Accepted with Revisions] SDL 0170 - SDL behavior in case of LOW_VOLTAGE event May 30, 2018
@theresalech
Copy link
Contributor Author

The Steering Committee has voted to accept this proposal with revisions. The revisions will include the use of 3 signals of the range SIGRTMIN - SIGRTMAX for LowVoltage, WakeUp and IgnitionOff - in place of using mqueue messages.

@theresalech
Copy link
Contributor Author

@LuxoftAKutsan please advise when a new PR has been entered to update the proposal to reflect the agreed upon revisions. I'll then merge the PR so the proposal is up to date, and enter issues in the respective repositories for implementation. Thanks!

@smartdevicelink smartdevicelink locked and limited conversation to collaborators May 30, 2018
@theresalech
Copy link
Contributor Author

Proposal has been revised to reflect revisions, and issues have been entered:
Core
RPC

@jordynmackool jordynmackool removed the rpc label Oct 22, 2018
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Projects
None yet
Development

No branches or pull requests

8 participants