Patch Set 3: Code-Review-1 (3 comments)
https://gerrit.osmocom.org/#/c/2786/3/src/common/oml.c File src/common/oml.c: Line 178: if (power <= 0) a negative or zero dBm value might be a bit unusual but well within the capabilities of most hardware and used in the context of e.g. testing or direct wired/cabled connections. get_p_max_out_mdBm() should be used in this context, with no fall-back. If it results in invalid values, it's a bug by osmo-bts. OML will reduce that power by max_pwr_red, but applying it prematurely here doesn't make sense. The terminology in the specs is unfortunately not always consistent. Line 187: msgb_tv_put(msg, NM_ATT_MANUF_STATE, power); why are we encoding the power in an IE that is called STATE ?!? Line 191: static inline int cleanup_attr_msg(struct msgb *msg, int length, uint8_t *out) * we generally have the output argument as first, just like memcpy() * we generlaly use 'const *' for input arguments * 'length' normally implies the length of the output buffer. If you mean an offset into the buffer, call it accordingly Also, an honest question: why do we pass attr_out_index+1 into the function, only to remove that byte here in the function body? -- To view, visit https://gerrit.osmocom.org/2786 To unsubscribe, visit https://gerrit.osmocom.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I9f72305bbf1ab74745bffac1bee9f539f5a6de32 Gerrit-PatchSet: 3 Gerrit-Project: osmo-bts Gerrit-Branch: master Gerrit-Owner: Max <msur...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder Gerrit-HasComments: Yes
