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

Powerd atom #89

Closed

Conversation

alfonsosanchezbeato
Copy link
Contributor

This atom implements a property that allows users to set low power mode for a modem. The API has been documented in doc/power-api.txt and a driver has been implemented for rilmodem.

Instructions to test this atom cam be found in
https://wiki.ubuntu.com/Process/Merges/TestPlans/ofono/LowPowerMode

@tonyespy
Copy link
Contributor

I have tested the code on mako / image #76 and everything seems to work as described.

I have noted one source comment inline, however my major issue is that adding a brand new core interface seems a bit heavy-handed for such a change as it really only exposes a single new property which allows a client to toggle the modem into low power mode.

Is there all that much difference in the semantics of your new LowPowerMode property and RadioSetting's FastDormancy property? After reading the description of FastDormancy in radio-settings-api.txt a few times, I think it's be fine to consider rilmodem's implementation the generic rilmodem approach, and that OEM implementations could could allow usage of better modem-specific requests ( eg. the FD requests used in your mtkmodem changes ). Leveraging this existing property would result in the least amount of changes to the ofono core.

If you have a strong objection to using FastDormancy, then my next choice would be to expose a property in an existing interface ( again RadioSettings seems an obvious choice ).

Another approach to avoiding core changes would be to expose this as a Touch ofono plugin, although I still think leveraging the existing FastDormancy property makes the most sense.

One other minor nit, is that the you the name "powerd" for the ATOM and for many of the file names, and this subconsciously implies that this is a private interface for use by Ubuntu Touch's "powerd" when it's really just a generic power interface.

@alfonsosanchezbeato
Copy link
Contributor Author

Ok, probably creating a new interface was over-kill, I already had the atom in the initial implementation that listened to powerd signals, so I just carried along with that.

Using the FastDormancy property is probably the best and easiest option, agreed. But, take into account that in the case of MTK, fast dormancy is in fact always enabled by rild. What happens is that there are two sets of timers, one which is applied when the screen is on and another one when the screen is off (the timeout to release radio resources is bigger when the screen is on). The FD request tells the modem which timer set to use. So we are kind of lying here when using this property with this name, for MTK SoCs. I cannot say for Qualcomm's. Anyway, as you see just having this property would not be enough to have a fully configurable FD interface, so I think it is fine to re-use it. In my opinion, it should in fact be renamed to something like LowPowerMode, making it more generic, as fast dormancy is a low-level network thing. However, I am not going to fight that battle with upstream ;-).

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

Successfully merging this pull request may close these issues.

None yet

2 participants