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

asha: import ASHA Pandora service from AOSP #309

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

asha: import ASHA Pandora service from AOSP #309

wants to merge 1 commit into from

Conversation

uael
Copy link
Contributor

@uael uael commented Oct 4, 2023

@@ -101,49 +104,59 @@ def on_audio_control_point_write(connection: Connection, value):
)
)

def on_read_only_properties_read(connection: Connection) -> bytes:
value = (
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this value expected to change during the lifetime of the characteristic? If not, why not construct it just once at init time?

+ bytes(AshaService.RESERVED_FOR_FUTURE_USE)
+ bytes(AshaService.SUPPORTED_CODEC_ID)
)
self.emit("read_only_properties", connection, value)
Copy link
Collaborator

Choose a reason for hiding this comment

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

The event name is not typical. Event names are normally nouns. The even here is that there is a read request on the characteristic. The event name should probably, then, just be 'read'.
But more important question: is this only for testing? Maybe the Attribute class itself should emit a 'read' event (like it emits a 'write' event), to make this observability more generally available?

return value

def on_le_psm_out_read(connection: Connection) -> bytes:
self.emit("le_psm_out", connection, self.psm)
Copy link
Collaborator

Choose a reason for hiding this comment

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

same comment about event names as above.

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.

2 participants