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

Create connected-network-type.yaml #158

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

gmuratk
Copy link
Contributor

@gmuratk gmuratk commented Jun 7, 2024

What type of PR is this?

Add one of the following kinds:

  • enhancement/feature

What this PR does / why we need it:

#143 requests this enhancement/feature as new API under Device Status.

Which issue(s) this PR fixes:

Fixes #143

Special notes for reviewers:

Proposal doesn't include the subscriptions portion.

Changelog input

 release-note

Additional documentation

This section can be blank.

docs

description: |
Client does not have sufficient permission.
In addition to regular scenario of `PERMISSION_DENIED`, other scenarios may exist:
- Phone number cannot be deducted from access token context.(`{"code": "NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT","message": "Phone number cannot be deducted from access token context"}`)
Copy link
Contributor

Choose a reason for hiding this comment

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

Seems like a little bit of NumberVerification 👀

{
  "code": "NUMBER_VERIFICATION.INVALID_TOKEN_CONTEXT",
  "message": "Phone number cannot be deducted from access token context"
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the comment. This is aligned with the other Device Status API endpoints. If there is a change needed, I can apply here as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hi @gmuratk,
which "other" DeviceStatus APIs are you referencing?
I do not see any mention of verifying a number within the access token in those APIs.

The only instance I found related to this is in the NumberVerification API.

As we are not currently expecting to extract and verify phone numbers from the access token for the DeviceStatus API, do you think we can proceed with a general 403 response in such cases?

Wdyt?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @maxl2287,
PR152, has the 2 files for separating the endpoints, and I used them as the basis for this yaml. Error message you pointed out exist in those files as well. https://github.com/camaraproject/DeviceStatus/pull/152/files

Connected-to-4G:
value:
lastStatusTime: "2024-02-20T10:41:38.657Z"
connectedNetworkType: 4G
Copy link
Contributor

Choose a reason for hiding this comment

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

4G you have not used here inside your enum:

  enum:
        - 2G
        - 3G
        - 4GLTE
        - 5GNSA
        - 5GSA
        - NON3GPP4G
        - NON3GPP5G
        - NOT-CONNECTED

Maybe you wanted to use 4GLTE ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

We are hearing of one use case more regularly "Companies want to check if a number is a VoIP number vs a nonVoIP number" , any chance we could get that as one of the options here?

- 5GSA
- NON3GPP4G
- NON3GPP5G
- NOT-CONNECTED
Copy link
Contributor

Choose a reason for hiding this comment

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

Change this to NOT_CONNECTED

- NON3GPP4G
- NON3GPP5G
- NOT-CONNECTED

Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the trailing spaces in line 177

@maxl2287
Copy link
Contributor

@camaraproject/device-status_codeowners I guess this can be moved together with #171 to the newly created https://github.com/camaraproject/DeviceQualityIndicator, do you agree?

@akoshunyadi
Copy link
Collaborator

@camaraproject/device-status_codeowners I guess this can be moved together with #171 to the newly created https://github.com/camaraproject/DeviceQualityIndicator, do you agree?

I think the quality indicator is something different. It will need different inputs, e.g. the network type, to provide a network quality indication.


ConnectedNetworkType:
description: |
* NOT_CONNECTED: The device is not connected to network
Copy link
Collaborator

Choose a reason for hiding this comment

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

I disagree with using NOT_CONNECTED. It should be UNKOWN (or NOT_KNOWN, or similar).

There are two reasons for this:

  • There are various reasons why the API provider might not know the network type, and not being connected to the network at all is just one of them. Rather than have separate codes for all these reasons, and the API provider trying to work out what the reason is, better just to say "I don't know".
  • The purpose of the API is to return the network type. Saying the device is not connected is providing information over and above this and, indeed, is information that is anyway provided by a different API (device-reachability-status). We should avoid having two APIs that do the same thing in different ways.

If there is an insistence that this API also returns information as to whether or not the device is connected to the network, then it should be an extension of device-reachability-status and not a new API.

Comment on lines +165 to +166
* NON3GPP4G: Device connected via non-3GPP Radio Access Technology but connected to 4G Network (e.g. WiFi Calling)
* NON3GPP5G: Device connected via non-3GPP Radio Access Technology but connected to 5G Network (e.g. WiFi Calling)
Copy link
Collaborator

Choose a reason for hiding this comment

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

There are two issues with separate NON3GPP4G and NON3GPP5G option as part of a single enum:

  • A device might be connected both to the mobile network and to WiFi for WiFi calling at the same time. Or it could be connected for WiFi calling and not connected to the mobile network. But we can only return one value.
  • For WiFi calling, I anyway don't see the value in distinguishing being connected to a 4G or 5G "network" (really, it is the core we are referring to here). Just knowing the device is additionally connected via WiFi should be sufficient.

I'd suggest to have a separate but optional WiFiConnected or WiFiCalling response field, which could be true, false or just not present if the API provider doesn't know or doesn't want to provide that information.

I appreciate that "WiFi" may one day become an outdated term, and other "non 3GPP" bearers may be possible, but the term is commonly understood (unlike "non 3GPP", which is a good example of the type of "telco-specific" language we are trying to avoid).

Comment on lines +163 to +164
* 5GNSA:4th Generation Mobile Communication Technology aka LTE with device connected 5G New Radio (NR)
* 5GSA: 5th Generation Mobile Communication Technology
Copy link
Collaborator

@eric-murray eric-murray Sep 27, 2024

Choose a reason for hiding this comment

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

It will be hard enough trying to work out what radio technology the device is using, without having to also work out which core it is connected to. I would suggest a single 5G option. What is the use case where the core technology is important?

* 5GSA: 5th Generation Mobile Communication Technology
* NON3GPP4G: Device connected via non-3GPP Radio Access Technology but connected to 4G Network (e.g. WiFi Calling)
* NON3GPP5G: Device connected via non-3GPP Radio Access Technology but connected to 5G Network (e.g. WiFi Calling)
type: string
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
type: string
* NTN4G: Device connected over 4G via 3GPP non-terrestrial network
* NTN5G: Device connected over 5G via 3GPP non-terrestrial network
type: string

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion above is to add non-terrestrial network (satellite) access, because it will incur increased propagation delay and reduced throughput vs. terrestrial 4G/5G

Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggestion above is to add non-terrestrial network (satellite) access

The problem with adding this option is that almost certainly nobody will support it. Why not? Because the device will almost certainly be roaming on such networks. And I don't see any API provider implementing a solution to determine what access technology a roaming device is using.

Non-terrestrial networks would be better detected using the roaming API, as they should be using a non-geographic PLMN.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Because the device will almost certainly be roaming on such networks

Ah, no, this is rather intended for the case where satellite is filling coverage gaps to regular UEs - in which case the PLMN ID is the same as the terrestrial network (so not roaming).

The same can apply to NB-IOT (i.e. terrestrial | LEO | GEO).

It may be cleaner to restrict the enum to the radio access type but include an additional field to indicate non-terrestrial access (default: false).

Copy link
Collaborator

Choose a reason for hiding this comment

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

... where satellite is filling coverage gaps to regular UEs

But you are still referring to a "direct to mobile" service using a 3GPP standard (4G or 5G)? Who is proposing to offer such a service using their normal terrestrial PLMN ID?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Who is proposing to offer such a service using their normal terrestrial PLMN ID?

Too early to say, but with reports of dozens of markets considering satellite-coverage deployment, then it makes sense to me to cover the valid 'same PLMN ID' option in the API (either as an enum value or with the optional modifier I mention above).

- 5GNSA
- 5GSA
- NON3GPP4G
- NON3GPP5G
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
- NON3GPP5G
- NON3GPP5G
- NTN4G
- NTN5G

Kevsy added a commit to Kevsy/APIBacklog that referenced this pull request Sep 27, 2024
Amended per API Backlog/Device Status discussion (ref Device Status [PR 158](camaraproject/DeviceStatus#158)
@akoshunyadi akoshunyadi marked this pull request as ready for review October 30, 2024 15:49
@gmuratk
Copy link
Contributor Author

gmuratk commented Nov 5, 2024

Thank you for all the comments over the past months. Since 'Draft' label is removed, we'll provide responses/edits soon.
cc: @murthygorty @RamTMO

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend: Device Status Connectivity with Standard
6 participants