libosmocore[master]: gsm0808: fix AoIP speech codec element parser/generator

[Neels Hofmeyr]

Patch Set 2: Code-Review-1

(10 comments)

(coding style comments, can't claim to have confirmed correctness with specs)

https://gerrit.osmocom.org/#/c/2820/2/src/gsm/gsm0808_utils.c
File src/gsm/gsm0808_utils.c:

Line 149: 	case GSM0808_SCT_FR1:
let's do it like this:

  bool type_extended;  (i.e. not setting a value at first)

  switch(sc->type) {
  case GSM0808_SCT_FR1:
  case GSM0808_SCT_FR2:
  case GSM0808_SCT_FR3:  (i.e. one combined case for all)
  case [...]:
          type_extended = false;
          break;
  case GSM0808_SCT_SCD:
          type_extended = true;
          break;
  default:
          OSMO_ASSERT(false);
          break;
  }

I'd prefer actual logging instead of ASSERT(false) but it seems this file has no logging anywhere; not sure whether that is policy, so accepting that.


Line 206: 	/* Note: If a configuration is present or not depends on the selected
This first sentence has no meaning? Ah, replace "If" with "Whether", please


Line 207: 	 * codec type. If present, it can either consist of one or two octet,
"octets"


Line 212: 	break;
please align the 'break' with the code, i.e. one more level of indent, to match general osmocom coding style.


Line 217: 	case GSM0808_SCT_FR5:
combine with FR4 case above


Line 221: 	case GSM0808_SCT_HR3:
combine with FR3 above


Line 224: 	case GSM0808_SCT_HR4:
again combine, same below. Better to avoid code dup, fine to give up alphabetic ordering.


Line 308: 	 * depending on the codec type */
"Whether ... octets"


Line 309: 	switch (sc->type) {
combine all similar cases, as above


https://gerrit.osmocom.org/#/c/2820/2/tests/gsm0808/gsm0808_test.c
File tests/gsm0808/gsm0808_test.c:

Line 591: 
(we usually avoid double blank lines ... maybe drop one that existed before this patch)


-- 
To view, visit https://gerrit.osmocom.org/2820
To unsubscribe, visit https://gerrit.osmocom.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: Idabb0f9620659557672e1c6b90c75481192e5c89
Gerrit-PatchSet: 2
Gerrit-Project: libosmocore
Gerrit-Branch: master
Gerrit-Owner: dexter <pmaier at sysmocom.de>
Gerrit-Reviewer: Jenkins Builder
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-HasComments: Yes