Conversation
Upstream recently (yesterday as of writing this patch) fixed the `thermal.h` header in https://android-review.googlesource.com/c/ platform/frameworks/native/+/3205910 to finally contain valid C code, allowing us to finally generate C bindings for this API and start writing safe bindings inside the `ndk` crate.
https://developer.android.com/ndk/reference/group/thermal `AThermal` allows querying the current thermal (throttling) status, as well as forecasts of future thermal statuses to allow applications to respond and mitigate possible throttling in the (near) future.
MarijnS95
commented
Aug 10, 2024
Comment on lines
+17
to
+25
| /// Workaround for <https://issuetracker.google.com/issues/358664965>. `status_t` should only | ||
| /// contain negative error codes, but the underlying `AThermal` implementation freely passes | ||
| /// positive error codes around. At least the expected return codes are "implicitly" documented to | ||
| /// be positive. | ||
| fn status_to_io_result(status: i32) -> Result<()> { | ||
| // Intentionally not imported in scope (and an identically-named function) to prevent | ||
| // accidentally calling this function without negation. | ||
| crate::utils::status_to_io_result(-status) | ||
| } |
Member
Author
There was a problem hiding this comment.
Unlike all other NDK APIs, this one returns positive errno values 🙃
MarijnS95
commented
Aug 10, 2024
Comment on lines
+75
to
+77
| /// # Warning | ||
| /// [`ThermalManager`] is synchronized internally, and its lock is held while this callback is | ||
| /// called. Interacting with [`ThermalManager`] inside this closure *will* result in a deadlock. |
Member
Author
There was a problem hiding this comment.
While all the extra locking makes it possible to add Send+Sync on ThermalManager, it means the user can now trivially pass this type into the callback and (if it's the same instance!) get a deadlock when interacting with it.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The header for this got fixed just yesterday to contain valid C so that we can parse it (without making trivial custom changes...) so now seems to be about the right time to actually write out the bindings and at least have them open in a PR!
https://developer.android.com/ndk/reference/group/thermal
AThermalallows querying the current thermal (throttling) status, as well as forecasts of future thermal statuses to allow applications to respond and mitigate possible throttling in the (near) future.