-
Notifications
You must be signed in to change notification settings - Fork 153
Bluetooth: Add Bluetooth BD address setup service and script #1212
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
base: master
Are you sure you want to change the base?
Conversation
- If controller returns Bluetooth Device Address (BDA) which is not
unique then device will set to unconfigured state, Hence add
`qca_set_bdaddr.service` and `qca_set_bdaddr.sh` to configure a
unique BDA on boot.
- Add `bluez5_%.bbappend` to:
- Include the new service and script in the BlueZ5 recipe.
- Include obex to support obex related profiles like OPP, FTP, MAP, PBAP
- Include only desired tools that are conditional on READLINE in bluez
- Install the script to `/etc/initscripts` and the systemd
unit to `bluetooth.target.wants`.
Signed-off-by: Janaki Ramaiah Thota <[email protected]>
koenkooi
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems an invasive enough change to be its own recipe and perhaps a bluez.bbappend to add the stand alone recipe to RRECOMMENDS.
| install -d ${D}${sysconfdir}/initscripts | ||
| install -m 755 ${UNPACKDIR}/qca_set_bdaddr.sh ${D}${sysconfdir}/initscripts/ | ||
|
|
||
| # Install service that will run qca_set_bdaddr.sh script on boot |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
systemd.bbclass should be enabling this by default already:
# Whether to enable or disable the services on installation.
SYSTEMD_AUTO_ENABLE ??= "enable"
[...]
if [ "${SYSTEMD_AUTO_ENABLE}" = "enable" ]; then
for service in ${@systemd_filter_services("${SYSTEMD_SERVICE_ESCAPED}", False, d)}; do
systemctl ${OPTS} enable "$service"
done
for service in ${@systemd_filter_services("${SYSTEMD_SERVICE_ESCAPED}", True, d)}; do
systemctl --global ${OPTS} enable "$service"
done
fi
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will reuse these SYSTEMD_AUTO_ENABLE.
|
|
||
| FILESEXTRAPATHS:prepend := "${THISDIR}/${BPN}:" | ||
|
|
||
| RRECOMMENDS:${PN} += " \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please keep variables sorted by usage, so R* goes below do_install, SRC* goes above do_unpack, configure vars go right above do_configure and installation vars go right above do_install.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the guidance. will reorder variables accordingly
| # SPDX-License-Identifier: BSD-3-Clause-Clear | ||
|
|
||
| # Function to format BD-Address and Set it | ||
| set_bda() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's zero error checking and handling in this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure will will error checking for each command.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the entire script actually
lumag
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also please don't describe patch contents in the commit message. Describe oly rationale and what is done to solve that.
| formatted_serial_number=$(echo "${hex_serial_number}" | sed 's/../&:/g;s/:$//') | ||
|
|
||
| # Append qualcomm general prefix BDA value 22:22 | ||
| BDA=22:22:${formatted_serial_number:0:14} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bashism, will not work with posix shells
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, where does 22:22 come from?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we took android HAL code as reference for this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't take random unreliable sources. There are standards, owners, etc.
| { | ||
| echo "public-addr $BDA" | ||
| sleep 1 | ||
| } | btmgmt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is it so complex? btmgmt --index 0 public-addr ${addr}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since btmgmt is interactive it is actually stuck If we directly use the command during boot in script . With above approach everytime it is working and it is not getting stuck.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please read carefully what I wrote.
| elif [[ "$unconfigured" == "DOWN RAW" ]]; then | ||
| break | ||
| elif [[ "$configured" == "DOWN" ]]; then | ||
| echo "BD-Address already configured!!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to say anything if it works as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noisy success messages Understood. will remove success prints
|
|
||
| RRECOMMENDS:${PN} += " \ | ||
| glibc-gconv-utf-16 \ | ||
| glibc-gconv-utf-32 \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add a comment, why do you need these
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
will handle in different recipe and PR with updated the comment
| " | ||
|
|
||
| #Include obex to support obex related profiles like OPP, FTP, MAP, PBAP | ||
| RDEPENDS:${PN} += "${PN}-obex" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is unrelated. In general, such things should be split into separate commits. In this particular case it should be solved by the distro layer instead by pulling the right set of packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will pull the commit changes related to obex tools to separate commit and PR
| #Remove desired tools from noinst-tools | ||
| NOINST_TOOLS:remove = " \ | ||
| ${@bb.utils.contains('PACKAGECONFIG', 'readline', '${INST_TOOLS_READLINE}', '', d)} \ | ||
| " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate commit with a proper explanation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, will handle in different PR
| #Install desired tools that upstream leaves in build area | ||
| for f in ${INST_TOOLS_READLINE} ; do | ||
| install -m 755 ${B}/$f ${D}/${bindir} | ||
| done |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be patched in the upstream or in OE-Core
Thanks! Agreed, will move the script and unit into a new recipe (qca-bt-bdaddr) and keep bluez5_%.bbappend separate. This keeps BlueZ packaging untouched and scopes the change properly. |
ok, will rework on the commit message scope and updated |
| @@ -0,0 +1,12 @@ | |||
| [Unit] | |||
| Description=start set BDA script for bluetooth | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please improve the description.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure, noted.
|
|
||
| [Service] | ||
| Type=simple | ||
| ExecStart=+/etc/initscripts/qca_set_bdaddr.sh |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nack, script should be an executable available in /usr/bin
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or ${sbindir}.
|
|
||
| # Function to check BD Address and wait until it becomes configured or unconfigured | ||
| validate_and_set_bda() { | ||
| while true; do |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer a fixed set of runs here, probably trying the loop for up to 5 times with a sleep 1 or similar between runs, no need for while true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure , will consider and update the iteration based on the results
| @@ -0,0 +1,52 @@ | |||
| inherit systemd | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please create a different recipe for this, it needs bluez but there is no need to be part of it.
qca_set_bdaddr.serviceandqca_set_bdaddr.shto configure a unique BDA on boot.bluez5_%.bbappendto:/etc/initscriptsand the systemd unit tobluetooth.target.wants.