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.

Sign up for GitHub

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

Jump to bottom

Move GetVendorId and GetProductId to DeviceInstanceInfoProvider #19514

Merged
merged 5 commits into from
Jun 15, 2022
Merged

Move GetVendorId and GetProductId to DeviceInstanceInfoProvider #19514

merged 5 commits into from
Jun 15, 2022

Conversation

kkasperczyk-no
Copy link
Contributor

@kkasperczyk-no kkasperczyk-no commented Jun 13, 2022 •
edited
Loading

Problem

GetVendorId and GetProductId are part of ConfigurationManager, while it can be useful to put those data into the factory data.

Change overview

  • Moved GetVendorId and GetProductId to DeviceInstanceInfoProvider to be able to easily override it for the factory data providers.
  • Added DeviceInstanceInfoProviderImpl for the platforms (Darwin, Linux, Android, Tizen, Webos) that has different methods implementation than LegacyDeviceInstanceInfoProvider.

@restyled-io restyled-io bot mentioned this pull request Jun 13, 2022
Closed
@kkasperczyk-no
Copy link
Contributor Author

kkasperczyk-no commented Jun 14, 2022 •
edited
Loading

Does any test coverage exist for these getters? If not, could some be added to TestConfigurationMgr.cpp? I see that it is using GetDeviceInstanceInfoProvider in TestConfigurationMgr_SerialNumber. Could this approach be duplicated to add some coverage?

The difference is that for serial number or other values tested in unit tests we also have "store" methods, so we can test store and then get value. In the product/vendor parameters case we have only getters, so there is not much to test. Nevertheless I added some basic tests checking the returned error code and making validation whether values fit the allowed limits.

bzbarsky-apple
bzbarsky-apple approved these changes Jun 14, 2022
View reviewed changes
Copy link
Contributor

@bzbarsky-apple bzbarsky-apple left a comment

Choose a reason for hiding this comment

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

As a followup, we should figure out what to do with StoreVendorId....

src/platform/Darwin/DeviceInstanceInfoProviderImpl.cpp Outdated Show resolved Hide resolved
@LuDuda LuDuda merged commit 09f3e4a into project-chip:master Jun 15, 2022
nicelyjust
nicelyjust reviewed Jun 30, 2022
View reviewed changes
src/platform/android/DeviceInstanceInfoProviderImpl.cpp
return err;
}

CHIP_ERROR DeviceInstanceInfoProviderImpl::GetProductId(uint16_t & productId)
Copy link
Contributor

Choose a reason for hiding this comment

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

@kkasperczyk-no HI Kamil ,in Android Platform, when you try to get the product ID, it cannot reached here, can you check that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicelyjust yes it looks like I did mistake when moving some methods to other modules. I pushed PR that I hope should fix this: #20208

kkasperczyk-no added a commit to kkasperczyk-no/connectedhomeip that referenced this pull request Jul 1, 2022
@kkasperczyk-no
[android] Fixed getting product id and product name
262567f 
In the project-chip#19514 there was a mistake done, as Android methods
were meant to only be moved to other module not change
their implementation.

Restored the previous implementation/
@kkasperczyk-no kkasperczyk-no mentioned this pull request Jul 1, 2022
Merged
kkasperczyk-no added a commit to kkasperczyk-no/connectedhomeip that referenced this pull request Jul 1, 2022
@kkasperczyk-no
[android] Fixed getting product id and product name
050ad35 
In the project-chip#19514 there was a mistake done, as Android methods
were meant to only be moved to other module not change
their implementation.

Restored the previous implementation/
andy31415 pushed a commit that referenced this pull request Jul 3, 2022
@kkasperczyk-no
[android] Fixed getting product id and product name (#20208)
08cb351 
In the #19514 there was a mistake done, as Android methods
were meant to only be moved to other module not change
their implementation.

Restored the previous implementation/
@nicelyjust nicelyjust mentioned this pull request Jul 12, 2022
Closed
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 13, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
6b99092 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
@ArekBalysNordic ArekBalysNordic mentioned this pull request Jul 13, 2022
Closed
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 14, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
2f1be7f 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 14, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
11e9eef 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 14, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
0ca0511 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 14, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
6652804 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 15, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
85c36cf 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 15, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
ef2f1bb 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 15, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
901212f 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 15, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
85f83d4 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
@ArekBalysNordic ArekBalysNordic mentioned this pull request Jul 15, 2022
Merged
ArekBalysNordic added a commit to ArekBalysNordic/connectedhomeip that referenced this pull request Jul 15, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation.
46d64b1 
In the project-chip#19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
woody-apple pushed a commit that referenced this pull request Jul 19, 2022
@ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation. (#20801)
fcd6594 
In the #19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
github-actions bot pushed a commit that referenced this pull request Jul 19, 2022
@ArekBalysNordic @web-flow
Set DeviceInstanceInfoProvider to the platform implementation. (#20801)
5b07bd1 
In the #19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.
@woody-apple woody-apple mentioned this pull request Jul 19, 2022
Merged
woody-apple added a commit that referenced this pull request Jul 19, 2022
@woody-apple @ArekBalysNordic
Set DeviceInstanceInfoProvider to the platform implementation. (#20801)…
56db839 
… (#20925)

In the #19514 there was a mistake made.
Within some platforms, the DeviceInstanceInfoProvider was set to
a generic one instead of the platform implementation.

To resolve this issue, the DeviceInstanceInfoProvider has been
set to the proper one in PlatformManager implementation for
Android, Darwin, Linux, Tizen, and WebOS.

Co-authored-by: Arkadiusz Bałys <arkadiusz.balys@nordicsemi.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nicelyjust nicelyjust nicelyjust left review comments

@msandstedt msandstedt msandstedt left review comments

@bzbarsky-apple bzbarsky-apple bzbarsky-apple approved these changes

@LuDuda LuDuda LuDuda approved these changes

@anush-apple anush-apple Awaiting requested review from anush-apple

@arkq arkq Awaiting requested review from arkq arkq is a code owner

@Byungjoo-Lee Byungjoo-Lee Awaiting requested review from Byungjoo-Lee

@carol-apple carol-apple Awaiting requested review from carol-apple

@chrisdecenzo chrisdecenzo Awaiting requested review from chrisdecenzo

@chshu chshu Awaiting requested review from chshu

@chulspro chulspro Awaiting requested review from chulspro

@Damian-Nordic Damian-Nordic Awaiting requested review from Damian-Nordic

@dhrishi dhrishi Awaiting requested review from dhrishi

@electrocucaracha electrocucaracha Awaiting requested review from electrocucaracha

@emargolis emargolis Awaiting requested review from emargolis

@erjiaqing erjiaqing Awaiting requested review from erjiaqing

@franck-apple franck-apple Awaiting requested review from franck-apple

@gjc13 gjc13 Awaiting requested review from gjc13

@harimau-qirex harimau-qirex Awaiting requested review from harimau-qirex

@hawk248 hawk248 Awaiting requested review from hawk248

@harsha-rajendran harsha-rajendran Awaiting requested review from harsha-rajendran

@isiu-apple isiu-apple Awaiting requested review from isiu-apple

@jelderton jelderton Awaiting requested review from jelderton

@jepenven-silabs jepenven-silabs Awaiting requested review from jepenven-silabs

@jmartinez-silabs jmartinez-silabs Awaiting requested review from jmartinez-silabs

@jtung-apple jtung-apple Awaiting requested review from jtung-apple

@kghost kghost Awaiting requested review from kghost

@kpschoedel kpschoedel Awaiting requested review from kpschoedel

@lazarkov lazarkov Awaiting requested review from lazarkov

@mlepage-google mlepage-google Awaiting requested review from mlepage-google

@mrjerryjohns mrjerryjohns Awaiting requested review from mrjerryjohns

@robszewczyk robszewczyk Awaiting requested review from robszewczyk

@sagar-apple sagar-apple Awaiting requested review from sagar-apple

@saurabhst saurabhst Awaiting requested review from saurabhst

@selissia selissia Awaiting requested review from selissia

@tcarmelveilleux tcarmelveilleux Awaiting requested review from tcarmelveilleux

@tecimovic tecimovic Awaiting requested review from tecimovic

@turon turon Awaiting requested review from turon

@vijs vijs Awaiting requested review from vijs

@vivien-apple vivien-apple Awaiting requested review from vivien-apple

@wbschiller wbschiller Awaiting requested review from wbschiller

@woody-apple woody-apple Awaiting requested review from woody-apple

@xylophone21 xylophone21 Awaiting requested review from xylophone21

Assignees
No one assigned
Labels
app darwin examples lib linux platform review - approved
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kkasperczyk-no @msandstedt @LuDuda @nicelyjust @bzbarsky-apple