Expose internal/private platform specific fields

[zerodea]

Expose platform specific fields to allow users to handle more advanced Bluetooth features after using Kable for discovery and/or connection

This change will allow users to implement their own advanced features, such as L2CAP, while the features are added to Kable, or even if the features are not added.

I haven't exposed JVM and JS fields because I'm not familiar with the platforms, and whether it is of interest. If it is I could research it and see what fields to expose, on JS it seems to be BluetoothDevice, but I haven't been able to figure it out at a quick glance for JVM (btleplug).

[twyatt]

I'm very hesitant to expose these fields because they are serious footguns.

Internally, Kable needs to keep state info for I/O and if a library consumer performs I/O directly w/ these exposed fields, then it will likely mess with the Kable machinery.

I understand that there are definitely use-cases where these risks are fine, but I'm afraid of getting very misleading/confusing bug reports due to misuse of these exposed fields.

I think it would be more appropriate to understand needed use-cases and implement the features that consumers needs vs. exposing this internal machinery.

[zerodea]

I understand and your hesitation makes sense, exposing internal fields is always a risk. I will keep working on the L2CAP PR. I'm working on it for a project and have a pretty clean Android implementation, but I still have to work on the iOS code and test both thoroughly.

[cedrickcooke]

@twyatt how would you feel about an opt-in annotation like InternalKableApi, with clear messaging about both the opportunity to create bugs and also no promise of API stability?

[twyatt]

@twyatt how would you feel about an opt-in annotation like InternalKableApi, with clear messaging about both the opportunity to create bugs and also no promise of API stability?

Ultimately, I want Kable to be useful for as many people as possible, so I would like to make fields available if they might help some consumers.

@InternalKableApi is a good idea to create some barrier at the code level.

My only remaining concern would be getting bug reports after someone used an "internal" API. So maybe we log a very loud log statement upon first access of an "internal" API? That way when they share logs it becomes very apparent that their problems may be from the API usage vs. a Kable bug. 🤷

[zerodea]

I have created an initial implementation of what the annotation could be like if that's the way you want to take it.

I also added another commit with a possible implementation of the log warning. It's a bit messy, but I couldn't think of a way to only show the message once and to only show it when the property is accessed from outside the Kable library. If you like the way the log message was handled I could add it to the other places where @KableInternalApi is being used.

I can also move the Logging to an object to have it a bit more organized instead of having a global variable and create a logger specifically for this so that you don't get the logger of the class you accessed it from, but instead something like "Kable/InternalApi" or whatever.

[twyatt]

I like this approach and appreciate the PR. I've been swamped with other projects at the moment, but hope to find some time to review this soon.

[zerodea]

I changed every usage of fields marked InternalApi so that it's easier to visualize what a more complete change might look like.
There are two odd things about these changes:

1. The usage of the fields becomes more uncomfortable internally because you have to remember to not use the public field and instead use the private or internal one which requires more care by people making changes and reviewing changes to the library
2. Because Logging is not Parcelable in ScanResultAndroidAdvertisement I made the Logging field null by default and if Logging is null it won't print the message. I think it's okay because most likely they will use InternalApi fields somewhere else too and see the message. We could maybe create the Logging instance in the displayInternalLogWarning function, but I I thought the tradeoff was better than breaking the usage for anyone using a custom LogEngine