RPi-Distro / pi-bluetooth

Loads BCM43430A1 firmware on boot
42 stars 34 forks source link

Should hciuart.service be forking? #13

Closed mibofra closed 4 years ago

mibofra commented 4 years ago

I've tested it manually, simply btuart doesn't fork. It starts, works, stops. The systemd service should be of type simple, not forking.

XECDesign commented 4 years ago

Well spotted, thanks. I haven't tested the change yet but committed it for the next release so I don't forget.

XECDesign commented 4 years ago

I don't think this is correct. I've just tried the change and it breaks bt. Without it, hciattach runs in the background. With it, it dies and bluetooth doesn't work.

mibofra commented 4 years ago

@XECDesign the thing is that it's not forking in my case, so it is not in background

mibofra commented 4 years ago

@XECDesign services disabled, if you launch manually btuart script, is it forking or not?

XECDesign commented 4 years ago

-n Don't detach from controlling terminal., which isn't given in btuart.

https://sources.debian.org/src/bluez/5.50-1.2%7Edeb10u1/tools/hciattach.c/#L1398

mibofra commented 4 years ago

@XECDesign

root@my-rpi:~# hciconfig 
root@my-rpi:~# btuart 
bcm43xx_init
Cannot open directory '/etc/firmware': No such file or directory
Patch not found, continue anyway
Set Controller UART speed to 3000000 bit/s
Device setup complete
root@parrot-rpi:~# hciconfig
hci0:   Type: Primary  Bus: UART
        BD Address: MYBDADDR  ACL MTU: 1021:8  SCO MTU: 64:1
        UP RUNNING 
        RX bytes:2200 acl:0 sco:0 events:130 errors:0
        TX bytes:3793 acl:0 sco:0 commands:130 errors:0

dmesg:

[   60.934236] Bluetooth: HCI UART driver ver 2.3
[   60.934256] Bluetooth: HCI UART protocol H4 registered
[   60.934336] Bluetooth: HCI UART protocol Three-wire (H5) registered
[   60.934631] Bluetooth: HCI UART protocol Broadcom registered

So at least in my case, it is not forking. That is why I say to you test it manually.

XECDesign commented 4 years ago

I've tested it in multiple ways. Documentation says it forks. Source code says it forks. Running it manually shows it forks. Running it from the service shows it forks. And at the end of the day bluetooth doesn't work with type=simple and then works when changed to type=forking, so I think it's fairly conclusive.

In your output, I don't see why you don't think it's forking. I think if you check ps aux, you will see that hciattach is running in the background.

mibofra commented 4 years ago

@XECDesign I've a doubt. Can you please output a systemctl status hciuart.service with simple and forking? I'm not talking about hciattach itself. Simply and probably, systemd doesn't leave hciattach alive because forks after the main process, but launching the script itself it's a simple serivce (the script starts and stops).

If you issue a status, you should see hciattach in the list of spawned services in forking at this point, and the opposite in simple. Sorry if it was the case.

pelwell commented 4 years ago

I think you can do that. You do have a Raspberry Pi OS installed, don't you?

mibofra commented 4 years ago

I think you can do that. You do have a Raspberry Pi OS installed, don't you?

Yes I've it too, let me check.

XECDesign commented 4 years ago

IIRC, the output is pretty much the same, except that the hciattach process gets killed after it forks when type=simple. Either way, I think we've spent enough time on it. There's not much need to go beyond what we've covered already. The change breaks bluetooth and the documentation and source both agree.

mibofra commented 4 years ago

@XECDesign I'm not talking about hciattach, it should be forking anyway. I'm talking about the behaviour of systemd towards the script. None is having doubts about hciattach forking after launching the script. That is why long time ago, when I was using directly hciattach in a service, the type was forking.

So sorry, with just simple, hciattach is no more in bg when systemd sees the script. I've not catched it. So yes, forking is a choice, another one is to put a RemainAfterExit=yes with type simple or oneshot, it will leave hciattach in bg (pidof finds the pid of hciattach).

Choice what you think it's nicer, the result is the same, anyway a possible restarting it will make the bluetooth unresponsive in both cases (for obviously reasons):

● hciuart.service - Configure Bluetooth Modems connected by UART
   Loaded: loaded (/lib/systemd/system/hciuart.service; enabled; vendor preset: enabled)
   Active: failed (Result: exit-code) since Wed 2020-08-19 16:58:10 CEST; 1min 19s ago
  Process: 1070 ExecStart=/usr/bin/btuart (code=exited, status=1/FAILURE)

ago 19 16:57:40 raspberrypi systemd[1]: Starting Configure Bluetooth Modems connected by UART...
ago 19 16:58:10 raspberrypi btuart[1070]: Initialization timed out.
ago 19 16:58:10 raspberrypi btuart[1070]: bcm43xx_init
ago 19 16:58:10 raspberrypi systemd[1]: hciuart.service: Control process exited, code=exited, status=1/FAILURE
ago 19 16:58:10 raspberrypi systemd[1]: hciuart.service: Failed with result 'exit-code'.
ago 19 16:58:10 raspberrypi systemd[1]: Failed to start Configure Bluetooth Modems connected by UART.

I'm just sorry I've didn't thought about the possibility of systemd killing hciattach after the main process, so sorry. I've reflashed the pi image to be sure, I was "playing" with the scripts, and even if I knew hciattach was forking, I've considered only the behaviour of the script, the thing was accepted, and I've not rechecked it again. RemainAfterExit=yes as said solve the issue, making the script closing freely and hciattach in bg.

XECDesign commented 4 years ago

No worries, systemd's behaviour can be a bit confusing sometimes.

I'm not talking about hciattach, it should be forking anyway. I'm talking about the behaviour of systemd towards the script.

That's one and the same. The type gives systemd a hint about how the binary behaves. In this case it forks and systemd needs to know that this is the expected behaviour. Maybe there are other ways which would also work, but this matches systemd's documentation the most. As I understand it, that's exactly what the forking type is for.

anyway a possible restarting it will make the bluetooth unresponsive in both cases (for obviously reasons)

IIRC, the chip needs to be power cycled to reset properly which isn't currently implemented. It's possible to fix, but I suspect it's not the highest priority right now.