search

microsoft / jacdac

Device and service catalogs for Jacdac.
https://aka.ms/jacdac
Creative Commons Attribution 4.0 International
66 stars 25 forks source link

rename service_number field everywhere #59

Closed tballmsft closed 3 years ago

tballmsft commented 3 years ago

I suggest we rename service_number to service_index, which more accurately reflects that the value of the field is an index into a device's advertised list of services.

jamesadevine commented 3 years ago

I think on one of our calls we opted for service_instance rather than service_index

pelikhan commented 3 years ago

Index is more accurate IMHO

From: James Devine notifications@github.com Sent: Thursday, November 12, 2020 7:24:22 PM To: microsoft/jacdac-ts jacdac-ts@noreply.github.com Cc: Peli de Halleux jhalleux@microsoft.com; Assign assign@noreply.github.com Subject: Re: [microsoft/jacdac-ts] rename service_number field everywhere (#86)

I think on one of our calls we opted for service_instance rather than service_index

— You are receiving this because you were assigned. Reply to this email directly, view it on GitHubhttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fmicrosoft%2Fjacdac-ts%2Fissues%2F86%23issuecomment-726255241&data=04%7C01%7Cjhalleux%40microsoft.com%7Cc3927f99583249bf1f1c08d887382de1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637408022650698410%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=w6Mm%2FNpmxq%2FY7Fvd76uEboc13GgejNlsF2Nc3sCT8yk%3D&reserved=0, or unsubscribehttps://nam06.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAA73QKOVR3KQMEROB4CU25DSPQR5NANCNFSM4TTTMX7Q&data=04%7C01%7Cjhalleux%40microsoft.com%7Cc3927f99583249bf1f1c08d887382de1%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C637408022650698410%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=oAtBf5idJqE%2BwOfJYfjyrrzncWZD%2FfcfMb7v2tABbfo%3D&reserved=0.

jamesadevine commented 3 years ago

Yeah, I have no issue with either, just reminding us what we decided on a call.

tballmsft commented 3 years ago

Sorry, I don't remember any of that. I prefer "index" over "instance".

jamesadevine commented 3 years ago

Yeah, me too! I think it was called that at some point...

tballmsft commented 3 years ago

one strange thing is the use of JD_SERVICE_NUMBER_CRC_ACK as a service number/index. Any reason this couldn't be a command in the control service?

jamesadevine commented 3 years ago

@mmoskal

tballmsft commented 3 years ago

IMO, we should adhere to the service abstraction interface as much as possible,

pelikhan commented 3 years ago

So "service_index"?

tballmsft commented 3 years ago

yes, "service_number" -> "service_index" everywhere.

mmoskal commented 3 years ago

JD_SERVICE_NUMBER_CRC_ACK (and also one for pipes) is an encoding trick to save bits. Service numbers only go up to 58 or so (because of packet size limits in announcement) but there's 5 bits reserved for it, so numbers 59-63 would otherwise go unused.

mmoskal commented 3 years ago

Oh, and also the CRC_ACK or PIPE messages are not tied to any particular service, so they don't need service index.

pelikhan commented 3 years ago

Renaming is done. I think the current conversation if for another bug.