-
Notifications
You must be signed in to change notification settings - Fork 12
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
Static Comms and Controller-Specific Init #58
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
These are inherently a part of the ExtensionController function set, and aren't used anywhere else in the library outside of this class and its children. Leaving these 'protected' at the moment until I can improve how these functions flow inside the class.
This was private, one line, and only called by reconnect(). Simplifying.
These all use the I2C reference for the object
This was more confusing than anything, since it's not clear to the user what the difference between "connect" and "reconnect" is. The "reconnect" function just didn't clear out the control data and connected id. But then again, why would you ever want to preserve the old control data if you're trying to reconnect to a controller? Having just the one function is simpler and more straight-forward.
This opens the door for controllers that need extra register writes before they'll communicate. This shouldn't cause an issue with existing programs since all examples have 'update' immediately after connect anyways.
For those handful of controllers and setups where extra register writes are required before the controller will communicate properly. At the moment that includes the Drawsome tablet and the NES controllers (acting in "high res" mode).
In hindsight I think this was a little misguided... By having these member functions publicly accessible outside of the normal control flow, the shared data is not updated as you would expect (notably with 'identifyController'). The user might also expect these to use the default I2C port instead of the member for the object, which is not necessarily intuitive. Instead, I'm making these protected so they are still available to children (see 'specialInit') and explicitly "for internal use only".
Now that these functions are for internal use only (62609ca), we can save the ID inside the function without affecting the comm flow.
Now that the controller-specific I2C functions are part of the ExtensionController class, this creates a name conflict (not literally because it's in the namespace, but still). Renaming this to solve that and make it more clear what this function does.
There's no reason for this to be private, and can be useful for checking whether the current ID matches what is expected.
Drawing a clearer distinction: the "ID" is the string of identifier bytes that mark the controller as unique. The "type" is the enumerated data that tells you what class or controller that corresponds to. Further, the "type" keyword also mirrors the "getControllerType" function.
Changed my mind on this. The keyword "special" makes it seem like it's somehow unique or its use is isolated to uncommon circumstances. In fact this is just a controller-specific initialization function, but as "controllerSpecificInit" seems too long "specificInit" will have to do.
Now elegantly handles multiple controller types even with controller-specific initializations (though none have been defined *yet*). I also added in a reconnect loop, because in hindsight the "quit" thing is a little weird.
dmadison
added a commit
that referenced
this pull request
May 26, 2020
The larger request size now needs to be set for each connection attempt. This was also broken by #58, which removed the initial update in the connect() function. That data was used for the "third party" check, so an update needs to be performed immediately after connection. Whoops.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR reworks the I²C communication setup: instead of being in the namespace, the controller-specific I²C functions are now in the
ExtensionController
class as static functions, with protected member functions that use thedata.i2c
member and have the ability to modify theExtensionData
when called.This serves two purposes:
Better encapsulation. The
ExtensionController
class is a high level wrapper for communication and data for every controller. It makes sense to include the controller-specific I²C functions as part of that interface.Originally I separated these out to prevent the class from becoming too "monolithic", but as they're simple and not used anywhere else in the library I think they definitely have their home here. Note that the lower level I²C functions are still a part of the
NXC_Comms.h
header.It allows derived classes (i.e. specific controllers) to write and read arbitrary data from the controller without having direct access to the I²C object.
This second point is necessary for the major feature of this PR: controller specific initialization.
The virtual function
specificInit()
has been added to the end of theconnect()
function, returning a boolean for whether the initialization is successful. Most controllers will use the original function inExtensionController
, but some controllers such as the Drawsome tablet (#57) require extra register writes before they'll communicate properly. Using a virtual function here lets each class specify its own extra initialization while maintaining encapsulation.To utilize this I've made the
controllerTypeMatches
(previouslycontrollerIDMatches
) function public so the user can easily check if the connected controller matches the one used by the derived class, and subsequently invokespecificInit()
. See theMultipleTypes
example for a demonstration of how this works.Since I'm already playing with the program's communication flow, I also decided to remove the
reconnect()
function. This was not well documented and it wasn't clear to the user how it was any different than the plain-oldconnect()
function (hint: the latter did not clear the control data). Why you would want to keep the control data if you're trying to reconnect to a controller whose data has presumably updated in the meantime, I don't know. It seemed prudent to make the distinction at the time, but having two nearly-identical functions with no explanation as to why seems more confusing than anything else. So now thereconnect()
function has been removed, all examples have been updated, and theconnect()
function will clear out the connected controller type and control data when called.Aaaand since I'm now changing the examples, I added a 1 second delay during reconnect attempts, both to prevent spamming the "disconnected" notice and to wait for the user to plug the controller back in (or fix that loose breadboard wire).
Last but not least, the namespace-scoped
identifyController()
function has been refactored todecodeIdentity()
, so as not to be confused with the class-scopedidentifyController()
function that polls the controller for its identity bytes and returns the enumerated controller type. Theextras
folder documentation has also been updated accordingly.