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

Move music control to Zbus and remove #ifdef #126

Merged
merged 3 commits into from
Nov 20, 2023
Merged

Conversation

ldab
Copy link
Collaborator

@ldab ldab commented Nov 20, 2023

ble_comm.c could be refactored and some of it moved to ble_gadgetbridge.c for example, music_control_event_callback looks a bit misplaced now, but it should work

#112 ?

@Kampi
Copy link
Collaborator

Kampi commented Nov 20, 2023

I´m currently on ble_comm.c to adjust the pairing process. So I can move some stuff somewhere else.

@jakkra
Copy link
Owner

jakkra commented Nov 20, 2023

I'm thinking if using zbus for stuff like this is necessary. It's not like this info needs to be propogated around the software, it just goes to either phone "backend". Maybe just a simple API is easier.
For example:
@ldab @Kampi What do you think?

// ble_comm.c
#define MAX_BACKENDS 2
ble_comm_backend_cbs backends[MAX_BACKENDS];

ble_comm_register_backend(backend_cbs);

ble_comm_music_control(action) {
    for (int i = 0; i < MAX_BACKENDS; i++) {
        if (ble_comm_backend_cbs[i].music_control) {
            ble_comm_backend_cbs[i].music_control(action);
        }
    }
}

// ble_comm_ios.c

static ble_comm_backend_cbs api = {
    .music_control = music_control,
    .send_battery = send_battery,
    ...
};

ble_comm_register_backend(api);

// ble_comm_gadgetbridge.c

static ble_comm_backend_cbs api = {
    .music_control = music_control,
    .send_battery = send_battery,
    ...
};

ble_comm_register_backend(api);

@ldab
Copy link
Collaborator Author

ldab commented Nov 20, 2023

I'm thinking if using zbus for stuff like this is necessary. It's not like this info needs to be propogated around the software, it just goes to either phone "backend". Maybe just a simple API is easier. For example: @ldab @Kampi What do you think?

// ble_comm.c
#define MAX_BACKENDS 2
ble_comm_backend_cbs backends[MAX_BACKENDS];

ble_comm_register_backend(backend_cbs);

ble_comm_music_control(action) {
    for (int i = 0; i < MAX_BACKENDS; i++) {
        if (ble_comm_backend_cbs[i].music_control) {
            ble_comm_backend_cbs[i].music_control(action);
        }
    }
}

// ble_comm_ios.c

static ble_comm_backend_cbs api = {
    .music_control = music_control,
    .send_battery = send_battery,
    ...
};

ble_comm_register_backend(api);

// ble_comm_gadgetbridge.c

static ble_comm_backend_cbs api = {
    .music_control = music_control,
    .send_battery = send_battery,
    ...
};

ble_comm_register_backend(api);

What is the downside of using zbus? I was under the impression that the thing was moving towards not using callbacks, but both solution works.

@jakkra
Copy link
Owner

jakkra commented Nov 20, 2023

Possible downside, maybe readability and more boilerplate code to define the zbus events etc.
But on the other hand zbus already handles the code needed in my proposal.

What do you think of just using one event for all phone communication, that will create less boilerplate zbus code. The data type can be an union with an ID? @ldab

@ldab
Copy link
Collaborator Author

ldab commented Nov 20, 2023

Possible downside, maybe readability and more boilerplate code to define the zbus events etc. But on the other hand zbus already handles the code needed in my proposal.

What do you think of just using one event for all phone communication, that will create less boilerplate zbus code. The data type can be an union with an ID? @ldab

I believe @Kampi is working on that on #112

@jakkra
Copy link
Owner

jakkra commented Nov 20, 2023

Ok, I'll merge this and then it can be cleaned up to make it same everywhere

@jakkra jakkra merged commit 04a468a into jakkra:main Nov 20, 2023
8 checks passed
@Kampi
Copy link
Collaborator

Kampi commented Nov 20, 2023

@jakkra sounds like a good idea. I had the idea already for BLE connected and disconnected event but it can be expanded with additional stuff. This can also provide some sort of API for the Bluetooth communication in other apps.

@jakkra
Copy link
Owner

jakkra commented Nov 20, 2023

@jakkra sounds like a good idea. I had the idea already for BLE connected and disconnected event but it can be expanded with additional stuff. This can also provide some sort of API for the Bluetooth communication in other apps.

Nice, BUT for the Bluetooth stuff, I think apps should use Zephyr API directly, it has good methods to get callbacks on connect etc. already. This should be used. We can add other stuff related to connections on zbus if we see a need.

@ldab ldab deleted the music-evt-zbus branch November 20, 2023 12:14
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

3 participants