Volume offset control service and client

[Thalley]

This PR implements the Volume Offset Control Service (VOCS) server and client.

The shell has purposely not been modified to control this server, as the service is a secondary service and only has purpose in the context of a primary service.

This PR is the first in a series of 3 to add the Volume Control Service server and client:

1. Add Volume Offset Control Service (VOCS) server and client (This)
2. Add Audio Input Control Service (AICS) server and client (Bluetooth: Audio: Audio Input Control Service and client #32785)
3. Add Volume Control Service (VCS) server and client (TBD)

The reason why VOCS and AICS should be implemented first, is that VCS (as a primary service) uses these two secondary services, and having them implemented first will make it easier to merge.

[Thalley]

At the time of this comment, the first 2 commits, 0a10231 and b628975, are commits from #31182 that are necessary to build. Once #31182 have been merged, these commits will be removed/ignored from this PR.

[asbjornsabo]

Some comments for consideration.
The question of the size of the audio location parameters has been submitted as an errata to the spec, and must be handled by GAWG

[asbjornsabo]

All my comments have been resolved to my satisfaction.
(I have not checked for other changes.)

[Thalley]

All my comments have been resolved to my satisfaction.
(I have not checked for other changes.)

Thanks @asbjornsabo - Now we just need 1 more review :)

[jhedberg]

Mostly coding style related comments, but also one functional issue regarding the meaning of data == NULL in callbacks.

[jhedberg]

Don't take this as a blocker/mandatory comment since it's completely subjective, but I wouldn't personally have taken the recent 80 -> 100 character limit change so that one must always strive for it. E.g. I find human readable text easier to read with shorter lines (less horizontal back-and-forth eye movement), so I'd still have formatted it to less than 80 characters. The main benefit of the 80 -> 100 chance (as I see it) is for code where forcing earlier line spits would actually hurt readability. Anyway, I realise this might also be tricky to configure in an editor so that it'd auto-split code and comments differently (that said, at least for code I write I always explicitly choose where to split the code, but let the editor do the splitting for any code comments or other documentation). (rant over :)

[Thalley]

I actually don't have any auto-split in my editor, so any such changes I've done has been intentional. Personally I don't find liken of length 100 to be any less readable, so it is, as you say, subjective :)

From: https://www.phoronix.com/scan.php?page=news_item&px=Linux-Kernel-Deprecates-80-Col

Yes, staying withing 80 columns is certainly still preferred. But it's not the hard limit that the checkpatch warnings imply, and other concerns can most certainly dominate

So from the Linux Kernel point of view, 80 should still be preferred. Going forwards I'll try to keep it at max 80, unless to avoid ugly breaks :)

[jhedberg]

I'd just do the following to avoid unnecessary indentation and long lines:

if (!data) {
        return BT_GATT_ITER_CONTINUE;
}

Actually, is this even a valid case? In what situation does this function get NULL as data?

[Thalley]

From gatt.h for bt_gatt_notify_func_t:

@param data Attribute value data. If NULL then subscription was removed.

I can change the checks from data == NULL to len == 0 as they (hopefully) always are paired like that.

w.r.t. doing

if (!cond) { 
    return;
}

I think that's a fine idea as it reduces indentation.

Though, to be fair, I've received opposed review comments on some of my other PR, where people dislike have returns in the middle of functions, so that's subjective :D But I'll update them to be if(!cond) as that is my personal preference as well.

[jhedberg]

I didn't realise NULL had special "subscription removed" meaning, rather than an actual indication/notification with empty payload. In that case checking for NULL is fine, but I'd do it the "early return" way along with a code comment about subscription removal.

[jhedberg]

Regarding preferences for single function return point, I think those might originate in some MISRA-C -related context, since there exists coding style requirements which state that every function must have a single point of return (I don't agree with it and I don't think it will ever be applied tree-wide).

[asbjornsabo]

Since the data pointer being NULL is the one specified in the documentation, I would rather trust that than trusting the length is also zero (the latter has not been specified).

[jhedberg]

I see these checks for data pointers repeated in many paces. When exactly does the stack give you NULL? When len is 0? I think it'd be more intuitive to just check for len in that case, which is what you're already doing, i.e. it's then redundant to check for data.

[Thalley]

See #31893 (comment)

[Thalley]

Actually, since this is for a read: If the client reads a characteristic, and the value returned by the GATT server has length 0, the data pointer will be NULL (and length as well). It's perfectly valid for the server to return 0 bytes from a read.

[jhedberg]

I guess it's subjective, but I think it'd look nicer if you had included some function parameters on the first line, and done the split after a parameter (i.e. after a , character). Same for the other function pointer typedefs

[Thalley]

Agreed. This was previously impossible due to the 80 line length (as some single parameters couldn't not fit on the line if starting at (, but it's possible now.

[jhedberg]

Empty line after } (unless the next line is also }. I think I saw at least one other place in the PR with the same issue. (minor thing, I know, but something we've tried to be consistent with within the Bluetooth subsystem)

[Thalley]

Agreed. This, imo, is however something a linter should handle for us, and not something you, as a reviewer, should need to deal with. Whatever linting we have that require empty line after a variable declaration: Can't that do this as well?

[jhedberg]

Probably, but I'm not sure this rule is followed much outside of the Bluetooth subsystem, so that might make it hard to enforce it on others. Linux checkpatch has support for subsystem-specific quirks, but I don't think we want to go that route with Zephyr. There are all sorts of small personal preferences which I feel like they help readability, but probably not worth codifying anywhere. Another which I've stopped pointing out about is the "reverse x-mas tree" rule for variable declarations where the shortest lines ones come last 😄

Even for this curly-brace rule, I'm not sure it applies in all cases, since the most important thing about empty lines is to group things which logically belong together into a single block. There could be cases where the line immediately following a block is so strongly related that adding the empty line detracts from easy understanding of the code. And that again means that automating this isn't really possible.

[Thalley]

I agree :) My personal opinion is that any code style things (such as empty lines after variable declaration, or code blocks) should be handled by a linter. A lot of PR reviews become longer due to such comments (although it's generally good that they are there!), which could have been fixed by the uploader before assigning reviewers :)

But as you also say, it's hard to apply such rules across the entirety of Zephyr as there are so many code styles applied in different subsystems.

[jhedberg]

Are you sure length == 0 always means unsubscription, and that it isn't e.g. possible for the remote to send an empty payload which would result with data != NULL && length == 0?

[Thalley]

You are completely correct here :) Removed the BT_DBG, but I guess we can't do much else here.

[jhedberg]

A few more coding style comments, hopefully you have patience with me being pedantic about this stuff 😄

[jhedberg]

So here the empty line really shouldn't be there. Assigning to a variable and checking for the variable's value is considered to be a single logical chunk of code.

[jhedberg]

What does data == NULL mean here? What is the stack trying to tell us?

[Thalley]

That there is no data; i.e. the GATT server sent invalid data on our read. This is application specific (some characteristics may be of length 0, so we cannot do anything about this below the application level).

[jhedberg]

How do you differentiate between GATT errors and errno? Positive value is GATT, negative is -errno? If so, please document it clearly. Same for all other callbacks with a similar parameter.

[Thalley]

Your assumption is correct: 0 = success, > 0 GATT and < 0 ERRNO. Updated.

[jhedberg]

Remove the empty line (assignment to variable and testing for the value)

[jhedberg]

Seems like you can probably remove err completely and just do return bt_gatt_write_without_response(...);

[jhedberg]

Here I wouldn't necessarily complain about the missing empty line after } since it's kind of a singular chunk of code, but you're actually not being consistent with yourself: there are other places such as the end of bt_vocs_client_location_get(), bt_vocs_client_state_set() and bt_vocs_client_description_get() where you do have the empty line, so I'd suggest to go with the same style here.

[Thalley]

Agreed; tried to update it after your last comment regarding this. Must've missed this one :)

[jhedberg]

Remove one of the empty lines

[Thalley]

A few more coding style comments, hopefully you have patience with me being pedantic about this stuff

Nah, it's great to get some fresh eyes on this :D

[jhedberg]

Looks good enough to me now thanks.

One thing which I don't feel strong enough to "request changes" anymore, that you might still want to do:

I wouldn't capitalize ERRNO since it's not an acronym or a literal API reference. It's referring to POSIX error numbers, and some POSIX APIs use the global (and thread unsafe) lower-case errno variable to store such values.

[Thalley]

Looks good enough to me now thanks.

One thing which I don't feel strong enough to "request changes" anymore, that you might still want to do:

I wouldn't capitalize ERRNO since it's not an acronym or a literal API reference. It's referring to POSIX error numbers, and some POSIX APIs use the global (and thread unsafe) lower-case errno variable to store such values.

That's a good point. I changed that too.

[joerchan]

This header file is using both bt_vocs and vocs as the prefix, they should probably be all bt_vocs.

[joerchan]

the input argument offset is not checked or used. Should we return invalid offset if it is non-zero?

[joerchan]

Should there be any handling of the return value here?

[Thalley]

See #31893 (comment)

[joerchan]

return error if offset is non-zero?

[joerchan]

Check return value?

[Thalley]

What do you suggest we do in case of error? Logging with BT_DBG or BT_WARN?

There's not really anything besides logging we can do as the server.

[joerchan]

I was mostly asking :) I suggest adding an explicit ignore of the return value.
(void)bt_gatt_notify_uuid(...)

[joerchan]

Can the application be both client and server? And would that cause a problem when you call location get and you get the client always?

Making this comment once, but it applies to all with the same pattern.

[Thalley]

Yes, the application can be both client and server. As per the API, setting conn = NULL will get the local server value, while conn != NULL will attempt to read the value from a remote server as the client. If this is unclear, we need to add more information in the documentation.

[joerchan]

I think that is a bit unclear, my suggestion would be to have two functions.
Local values for the server should be _get/_set, and the clients _read/_write to show the GATT operation.