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

Align System Capability Manager Behavior with Java Suite #1543

Merged
merged 41 commits into from Mar 3, 2020

Conversation

joeljfischer
Copy link
Contributor

@joeljfischer joeljfischer commented Feb 4, 2020

Fixes #1535

This PR is ready for review.

Risk

This PR makes minor API changes.

Testing Plan

  • I have verified that I have not introduced new warnings in this PR (or explain why below)
  • I have run the unit tests with this PR
  • I have tested this PR against Core and verified behavior (if applicable, if not applicable, explain why below).

Unit Tests

  • New subscribe method
  • isCapabilitySupported method

Core Tests

  • Test Obj-C subscribe with 0, 1, 2, and 3 parameters

    • Test cached capability
    • Test updated capability
    • Test unsubscribe
  • Test Obj-C subscribe with update handler

    • Test cached capability
    • Test updated capability
    • Test unsubscribe
  • Test Obj-C subscribe with block (deprecated)

    • Test cached capability
    • Test updated capability
    • Test unsubscribe
  • Test isCapabilitySupported

  • Check method signatures for Swift

Core version / branch / commit hash / module tested against: Sync Gen 3.2 (18124_DEVTEST), Manticore v.2.4.2 (SDL Core v6.0.1)
HMI name / version / branch / commit hash / module tested against: Sync Gen 3.2 (18124_DEVTEST), Manticore v.2.4.2 (Generic HMI v0.7.2)

Summary

This PR makes additions and some minor behavioral changes to existing system capability manager methods in order to bring behavior into alignment with the Java Suite SCM as much as is reasonable.

Changelog

Enhancements
  • Added a SystemCapabilityManager.subscribeToCapabilityType:withUpdateHandler: method with more verbose error reporting than the now deprecated SystemCapabilityManager.subscribeToCapabilityType:withBlock: method.
  • The SystemCapabilityManager.subscribeToCapabilityType:withObserver:selector: selector now supports up to three parameters, adding an error parameter and boolean subscribed parameter.
  • All subscribe methods now immediately return the cached value.
  • Added isCapabilitySupported: method that can be used to quickly determine if the feature is supported by the head unit or not.
Bug Fixes
  • All capability data is no longer downloaded on first HMI_FULL. Instead the data will be downloaded when the first updateCapabilityType:completionHandler: call or the first subscribe to that capability type is made.
  • Calling unsubscribeFromCapabilityType:withObserver: such that no observers remain for that type will now unsubscribe the SystemCapabilityManager from receiving any more data about that type until updateCapabilityType:completionHandler: or another subscribe call is made for that type.
  • Removed caching data from GSC responses sent outside of the SCM. This simplifies the manager, tests, and aligns with the Java Suite SCM.

CLA

@joeljfischer joeljfischer added best practice Not a defect but something that should be improved anyway manager-system-capability Relating to the manager layer - system capability labels Feb 4, 2020
@joeljfischer joeljfischer self-assigned this Feb 4, 2020
@joeljfischer joeljfischer added this to In progress in v6.6.0 via automation Feb 4, 2020
@joeljfischer joeljfischer linked an issue Feb 4, 2020 that may be closed by this pull request
@joeljfischer joeljfischer changed the title WIP: Align System Capability Manager Behavior with Java Suite Align System Capability Manager Behavior with Java Suite Feb 5, 2020
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

SmartDeviceLink/SDLSystemCapabilityManager.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSystemCapabilityManager.h Outdated Show resolved Hide resolved
v6.6.0 automation moved this from In progress to Waiting for Review Feb 10, 2020
* Fix the displaysObserver not being properly generated
* Add tests around non-DISPLAYS subscribed bool for subscriptions with 3 selector parameters
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

SmartDeviceLinkTests/SDLSystemCapabilityManagerSpec.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLError.m Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSystemCapabilityManager.h Show resolved Hide resolved
SmartDeviceLink/SDLSystemCapabilityManager.h Show resolved Hide resolved
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments

SmartDeviceLink/SDLSystemCapabilityManager.m Show resolved Hide resolved
SmartDeviceLink/SDLSystemCapabilityManager.m Show resolved Hide resolved
* Fix HMI status is never updated
* Fix HMI NONE error is now propagated to the observer
v6.6.0 automation moved this from Waiting for Review to Reviewer approved Feb 20, 2020
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Documentation changes requested

SmartDeviceLink/SDLSystemCapabilityManager.h Outdated Show resolved Hide resolved
v6.6.0 automation moved this from Reviewer approved to Waiting for Review Feb 20, 2020
Copy link
Contributor

@NicoleYarroch NicoleYarroch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some comments on docs

SmartDeviceLink/SDLSystemCapabilityManager.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSystemCapabilityManager.h Outdated Show resolved Hide resolved
SmartDeviceLink/SDLSystemCapabilityManager.h Outdated Show resolved Hide resolved
v6.6.0 automation moved this from Waiting for Review to Reviewer approved Feb 20, 2020
@joeljfischer joeljfischer merged commit 73d8dcf into develop Mar 3, 2020
v6.6.0 automation moved this from Reviewer approved to Done Mar 3, 2020
@joeljfischer joeljfischer deleted the bugfix/issue-1535-align-systemcapabilitymanager branch March 3, 2020 14:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
best practice Not a defect but something that should be improved anyway manager-system-capability Relating to the manager layer - system capability
Projects
No open projects
v6.6.0
  
Done
Development

Successfully merging this pull request may close these issues.

Update SystemCapabilityManager to align with Java_Suite
3 participants