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

It's impossible using Bluejay with some other libraries #119

Closed
larryonoff opened this issue Nov 9, 2017 · 17 comments · May be fixed by #227
Closed

It's impossible using Bluejay with some other libraries #119

larryonoff opened this issue Nov 9, 2017 · 17 comments · May be fixed by #227

Comments

@larryonoff
Copy link
Contributor

Bluejay does the right thing and incapsulates CoreBluetooth classes. But in my case I have to use DFU library, https://github.com/NordicSemiconductor/IOS-Pods-DFU-Library that requires direct access to CBCentralManager and CBPeripheral. As result I had to get rid of all Bluejay super-features and implement manually a part of work that Bluejay does perfectly.

Maybe it would be reasonable to expose some CoreBluetooth classes at developers risk? So it would be some-kind of Force Unwrap in Swift?

@sakuraehikaru
Copy link

I like the idea of providing flexibility. A total black box isn't always ideal. But I am not sure how best to expose the CBPeripheral and CBCentralManager references so that we don't encourage less experienced developers to use them, nor easily allow them to stumble upon those.

Thoughts @nbrooke or @larryonoff?

@nbrooke
Copy link
Member

nbrooke commented Nov 9, 2017

I'm not sure if it's possible to support two libraries actually sharing the same CBCentralManager even in theory. Since there is only a single delegate on CBCentralManager and you need to install the delegate to do anything useful, letting one library do anything will necessarily damage the state of the other one. You'd need to essentially shut down one while using the other.

Just doing that yourself in the app (Having two CBCentralManagers) would be one option, but I imagine it would be useful to be able to do some work with he more convenient API of Bluejay (connect to the peripheral, check firmware version before you start the update) and then pass off the connected peripheral to the other library when you are ready to get started.

What we COULD potentially do to allow that without too much complexity or danger is to provide a way to start a Bluejay instance with an existing CBCentralManager (and connected CBPeripheral) and a way to do a purposeful "non-clean" shutdown that resets all internal state, but does not disconnect and returns rather then deallocating the active CBCentralManager and CBPeripheral . So usage would look something like this (lots of spread between different functions and async behaviour elided for clarity):

// Setup bluejay and do some operations
let bluejay = Bluejay()
bluejay.start(connectionObserver: self, backgroundRestore: .disable)
bluejay.connect(/* ... */)
bluejay.read(/*...*/

// Later:
// Stop Bluejay and extract existing CoreBluetooth state with new API
let coreBluetooth: (manager: CBCentralManager, connected: CBPeripheral?) = bluejay.stopAndExtractBluetoothState()
// Bluejay is now back in pre-start state, no manager, no connectedPeripheral, no internal state, any calls will trap

let otherLibrary : OtherLibrary? = OtherLibrary(coreBluetooth.manager, coreBluetooth.connected)
otherLibrary?.doStuff()
otherLibrary = nil // other library destructed here and stops using manager and peripheral

// Restart bluejay with the existing CB objects using new parameter to start (could either use existing instance or allocate a new one)
bluejay.start(connectionObserver: self, backgroundRestore: .disable, cbState: coreBluetooth)

I don't think that either of the new "stopAndExtractState" (should probably have a better name) or the alternative form of "start" are particularly hard to implement, and I think would enable what you are trying to do without exposing internal state in a way that could be misused via attempting to actually use the CB objects at the same time Bluejay is.

@larryonoff
Copy link
Contributor Author

I like the idea. I think that API may look in the following way

struct CoreState {
  let centralManager: CBCentralManager
  let connectedPeripheral: CBPeripheral?
}

extension Bluejay {
  func extractCoreState(shouldStopBluejay: Bool = true) throws -> CoreState
  func start(..., coreState: CoreState? = nil)
}

@larryonoff
Copy link
Contributor Author

There's one more idea. Considering Bluejay as a framework that connects to CoreBluetooth, like

let centralManager: CBCentralManager = ...
...


let bluejay = Bluejay(centralManager: centralManager)

// sets CBCentralManagerDelegate and other required stuff
//  may store current centralManager.delegate
bluejay.start()

// restore initial centralManager.delegate 
// stops / pauses all Bluejay jobs
bluejay.pause() / bluejay.stop() 

// attempts to resume according to the latest known Bluejay state
bluejay.resume()

@nbrooke
Copy link
Member

nbrooke commented Nov 10, 2017

I'm not wild about either of those alternate approaches.

Like I said at the outset, i don't think it's possible to share the manager object between libraries that have any internal state at all safely. If you ever change the delegate while there are in flight operations, then callbacks will not get delivered to the library that is expecting them, or callbacks that the library is not expecting will get delivered, and once that has happened the internal state is damaged. This is particularly bad for Bluejay because it's whole metaphor is trying to impose reliability by being VERY specific about the queuing of operations so we (try to) never end up in a state we don't understand, allowing swapping of it's CBCentralManger delegate without cleaning all internal state is very likely to fail in cases where there is any operation in progress either when you start or stop.

I think both these proposed tweaks with the shouldStopBluejay flag in the first and the pause that maintains internal sate in the second, are WAY more error prone than having the extract state always explicitly stop.

Thinking about it more, I would actually argue that if we REALLY wanted to be safe:

  • Calling stopAndExtractState when there are any operations in the queue should probably be an error, or it should be asynchronous and wait for all outstanding operations to complete (or wait for the first operation in the Queue to complete and cancel all the others?), since outstanding callbacks delivered to the other library might cause them problems as well,
  • Calling start with a CBCentralManager or CBPeripheral that already has a delegate should be an error
  • We should maybe verify on every connect/disconnect/read/write call that the CBCentralManager and CBPeripheral delegates haven't been changed (this is probably overkill, but it's definitely a possible failure mode once you allow attaching to a pre-existing CBCentralManager if the caller hold on to that reference and passes it to another library after giving it to us)

@DrAma999
Copy link
Contributor

DrAma999 commented Feb 6, 2018

I'm probably going to have the same issue, the nordic library, to start a firmware update, requests the CBCentralManager and the CBPeripheral, looking at the source code it seems to take over any operation by changing their delegates.
I'm wondering if would be enough to just expose those properties in Bluejay, of course bluejay would not receive callbacks anymore, but as far as I understand after an update the device will be reset and connection would be lost and then restored.
Once nordic has been done all its stuff Bluejay could take control again. I tried to lookup the source code and it seems that all of the states API are just getter around CBManager and CBPeripheral.
At this point I'm wondering what internal states does bluejay keep. Let say that the queue of the requests is empty, let say that we could remove all the observer and them later, am I missing something else?

@nbrooke
Copy link
Member

nbrooke commented Feb 13, 2018

Having those exposed would definitely be enough to implement this sort of thing, and yes, Bluejay does not have much significant state if there are no objects in the queue (thought there is some, the fact that the peripheral is connected is a bit of implicit state that could be invalidated in a scenario like this)

So when there aren't any object in the queue, you definitely could just swap the delegate pointers and then put them back later.

The reason I'd prefer the more heavyweight style of interface with explicit stop and start calls as discussed above is that just exposing those pointers in general could open up lots of API surface for potential mistakes (swapping the delegates when the queue is NOT empty, making other calls on those objects that would cause callbacks when Bluejay isn't expecting them, etc.) I think a stop call that returns the current objects and a start call that takes the objects provides an interface that let's people use the library in this way, without introducing any potential dangers.

We (Steamclock) will definitely make this change when we have some time to work on some non-essential features, but because it's not something we need for any of our projects, I don't want to promise any particular timeline. A pull request that implemented this along the lines of the stop and restart model would definitely be appreciated, if anyone else is willing to give it a shot.

@DrAma999
Copy link
Contributor

thank you @nbrooke , I've forked the project and I already made some changes. Now I'm modifying the sample project, to test if it working correctly. When it will be done I will tell you and maybe I can make a pull request.

@DrAma999
Copy link
Contributor

DrAma999 commented Apr 4, 2018

We created a fork of Bluejay after some testing I can tell you that is working pretty fine along with nordic DFU, you can connect to a device and if an update is needed you can pass the peripheral and the manager to nordic library. After the update is completed you can give back those to BlueJay.

@nbrooke
Copy link
Member

nbrooke commented Apr 6, 2018

That change looks good to me. I don't know if we would want to take the change to the sample code (that's a lot of complexity added to the sample code for something that is a more advanced feature, might make more sense to have a separate test project for demoing this or something), but the change to Bluejay itself looks great.

@jeremychiang Let's get this merged in next time you are doing Bluejay updates.

@DrAma999
Copy link
Contributor

awesome. I'm really grateful that you integrated this feature

sakuraehikaru pushed a commit that referenced this issue May 1, 2018
* Add the stop and extract API, #119

* Bump version to 0.6.0
@sakuraehikaru
Copy link

Implemented.

@MetalManipulator
Copy link

We created a fork of Bluejay after some testing I can tell you that is working pretty fine along with nordic DFU, you can connect to a device and if an update is needed you can pass the peripheral and the manager to nordic library. After the update is completed you can give back those to BlueJay.

I apologize if this is the wrong location to ask my question, but I wanted to quote @DrAma999 's efforts. Please guide me to the correct location if this is incorrect.

I am attempting to do the same Nordic DFU flow. I have got it all working except for cleanly handing control back from CB to Bluejay. Bluejay reports that it has started when I give it the CBCentralManager reference only, but once I attempt my first operation back with Bluejay I get "Queue is paused because CBCentralManager is not ready yet".

Is there any checking or flushing that needs to be done to CB before handing it back to Bluejay? Would @DrAma999 be willing to share a snippet of their sample code displaying the success of restarting Bluejay after the DFU operation?

Thank you

@sakuraehikaru
Copy link

@MetalManipulator I might have an idea of what the problem is.

@DrAma999's use case might be Bluejay first, then Nordic, then back to Bluejay. Yours might be Nordic first, then back to Bluejay.

The bug might be that if you don't start with a Bluejay instance first, then the Bluejay queue won't get started. The queue is usually started by listening to centralManagerDidUpdateState, and if the queue is not started, then you would hit the CBCentralManager is not ready yet block.

The fix might look like:

case .use(let manager, let peripheral):
    cbCentralManager = manager

    if let peripheral = peripheral {
        connectedPeripheral = Peripheral(delegate: self, cbPeripheral: peripheral)
        peripheral.delegate = connectedPeripheral
    }

    // Fix here:
    queue.start()
}

@MetalManipulator
Copy link

@jeremychiang I apologize if it wasn't clear before. I actually am starting with Bluejay, moving to Nordic, and then back to Bluejay. The Nordic framework finishes by disconnecting from the device, so I am trying to restart Bluejay with just the previous CBCentralManager reference I got when I stopped Bluejay before and not providing a CBPeripheral since I'm not connected.

I will look further into your suggestion above though to see if I can get the queue going. Thank you.

@DrAma999
Copy link
Contributor

DrAma999 commented Oct 2, 2019

There is bug, I couldn't discover it because for our business requirements I needed to reinstate another instance of bluejay after updating the firmware.
Basically what is missing is restoring the delegate on the CBCentralManager:

case .use(let manager, let peripheral):
            cbCentralManager = manager
            // here is missing cbCentralManger.delegate = self
            if let peripheral = peripheral {
                connectedPeripheral = Peripheral(delegate: self, cbPeripheral: peripheral)
                peripheral.delegate = connectedPeripheral
            }
        }

I don't know if they also changed something in the Nordic library that could cause some other issues.

@MetalManipulator
Copy link

@DrAma999 Thank you for your insight. I will proceed accordingly.

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 a pull request may close this issue.

5 participants