search

scratchfoundation / scratch-link

Device interoperability layer for Windows and MacOS
BSD 3-Clause "New" or "Revised" License
102 stars 83 forks source link

Add `getVersion` to Scratch Link protocol #145

Closed cwillisf closed 5 years ago

cwillisf commented 5 years ago

Proposed Changes

Adds a getVersion command to all session types. This command retrieves version information -- in particular, the protocol version implemented by Scratch Link.

Because of the new protocol feature I've bumped the protocol version to 1.3; if getVersion fails then the caller may assume that it's connected to a copy of Scratch Link implementing protocol version 1.2 or below.

I want to stress that this is the protocol version and not the application version of Scratch Link. Presumably other applications may implement this protocol; those applications should report the version of the protocol they implement rather than the application version.

This new feature is also documented in NetworkProtocol.md. I left it fairly open-ended so that we can (for example) add the application version later if we discover that it would help.

Reason for Changes

This will allow extensions to adjust their communication with Scratch Link to use or avoid new or old features if necessary. Hopefully there won't be much occasion for this but it's better to have getVersion and not need it than the other way around.

Note to reviewers

I recommend hiding whitespace changes for BTSession.swift since otherwise you'll see that almost every line in BTSession.swift's didReceiveCall method has changed indentation.

cwillisf commented 5 years ago

It occurred to me this morning that since we haven't yet released a version of Scratch Link which implements protocol version 1.2, it should be OK to add the getVersion feature into protocol version 1.2 instead of bumping to 1.3. That could be beneficial for extensions which need manufacturerData: if getVersion and manufacturerData are both in 1.2 then an extension can be sure that a getVersion failure means that manufacturerData is unavailable, whereas if getVersion is a 1.3 feature then a getVersion failure means manufacturerData may or may not be available.

Any objections? If I don't hear any then I will "un-bump" and merge getVersion into protocol version 1.2 later today.

paulkaplan commented 5 years ago

@cwillisf LGTM. I don't see pingMe in the network protocol documentation, is that something that is just used for debugging or is it used in any client?

cwillisf commented 5 years ago

@paulkaplan just for debugging... I was thinking about removing it but it's kind of nice for use in the playground :)