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.
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
Cwbollinger/mobile api wip #69
base: master
Are you sure you want to change the base?
Cwbollinger/mobile api wip #69
Changes from 8 commits
6a34329
e107d82
02b4c54
9c2042a
1ebd059
05e84bc
52a8202
5821d60
03c9ef4
55e0318
58b1356
a7e2b4b
ac722bf
76348e1
2127df2
6685146
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think these have
bool
return values; I'm not sure about the balance between making the example more complex vs. more robust by handling themThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just looking at the example here vs. the code below, it isn't clear what happens if it can't find these modules on the network. I'd probably match the arm and mobile IO and have a static
create
method that returns aunique_ptr
, so we can have a invalid option.(actually...if we are OK bumping to C++17, returning an
optional
might be a good way to go...but we'd want to update the MobileIO and Arm APIs too. Worth further discussion...)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be addressed now, and be c++11 compatible
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This reminds me...I think we still need to fix the
MobileIO::getState
call to not block in C++, or accept a timeout. I'll track this as a separate issue.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really should add a CMake function here to help clean this and the other content up. I'm adding a issue for this post-merge.