Change in libosmocore[master]: add GAD coding for Location Services

[neels]

neels has posted comments on this change. ( https://gerrit.osmocom.org/c/libosmocore/+/20332 )

Change subject: add GAD coding for Location Services
......................................................................


Patch Set 1:

(5 comments)

thx, will apply...

https://gerrit.osmocom.org/c/libosmocore/+/20332/1/src/gsm/gad.c 
File src/gsm/gad.c:

https://gerrit.osmocom.org/c/libosmocore/+/20332/1/src/gsm/gad.c@37 
PS1, Line 37: 	OSMO_VALUE_STRING(OSMO_GAD_TYPE_ELL_POINT),
> probably want to have this as "ELL_POINT", etc. […]
that's the style you guys use, yes. I really much much prefer to have in the log output the exact actual names that things have in the source code, because it makes working with the source so much easier. If the names are shortened, it adds another indirection every single time of first looking up the "weird" names in the value_string[], and only then can I grep the sources.

If someone else adds to pespin's opinion I'll change it (and not like it, but ok)


https://gerrit.osmocom.org/c/libosmocore/+/20332/1/src/gsm/gad.c@55 
PS1, Line 55: uint32_t osmo_gad_enc_lat(int32_t lat_deg_1e6)
> Missing documentation in all functions. I think it is really needed here.
ack


https://gerrit.osmocom.org/c/libosmocore/+/20332/1/src/gsm/gad.c@162 
PS1, Line 162: #ifdef GAD_FUTURE
> explain this?
i'll add a comment


https://gerrit.osmocom.org/c/libosmocore/+/20332/1/src/gsm/gad.c@250 
PS1, Line 250: 	*gad = (struct osmo_gad){};
> what's the point of memzeroing here?
reproducability in gad_test.c, which allows a memcmp(sizeof(struct osmo_gad))


https://gerrit.osmocom.org/c/libosmocore/+/20332/1/src/gsm/gad.c@293 
PS1, Line 293: 		OSMO_STRBUF_PRINTF(sb, "Ellipsoid-point-with-uncertainty-circle{to-str-not-implemented}");
> all of these look like they should mostly be calls to get the value from value_string....
ok, can add such, but there still needs to be a switch() for the diverse elements to be printed.
So there will be a switch() here, *plus* an array iteration lookup.

Below points should also get their individual elements printed at some point, just so far osmo-smlc is only composing an ell point unc circle type, so i skipped the monkey work for now.



-- 
To view, visit https://gerrit.osmocom.org/c/libosmocore/+/20332
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Change-Id: I7a9dd805a91b1ebb6353bde0cd169218acbf223c
Gerrit-Change-Number: 20332
Gerrit-PatchSet: 1
Gerrit-Owner: neels <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: neels <nhofmeyr at sysmocom.de>
Gerrit-CC: laforge <laforge at osmocom.org>
Gerrit-CC: pespin <pespin at sysmocom.de>
Gerrit-Comment-Date: Thu, 01 Oct 2020 13:04:52 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: pespin <pespin at sysmocom.de>
Gerrit-MessageType: comment
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20201001/6d9dc127/attachment.htm>