Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/11361 )
Change subject: cardem: add more debug information for TPDU state ...................................................................... Patch Set 1: Code-Review+1 (2 comments) https://gerrit.osmocom.org/#/c/11361/1/firmware/libcommon/source/card_emu.c File firmware/libcommon/source/card_emu.c: https://gerrit.osmocom.org/#/c/11361/1/firmware/libcommon/source/card_emu.c@110 PS1, Line 110: OSMO_VALUE_STRING(TPDU_S_WAIT_CLA), normally I'm not a big fan of OSMO_VALUE_STRING as the auto-generated string names are often way too long. However, in this case, the enum name is rather short and we can keep it. Iwould consider "WAIT_INS" to be shorter but equally informative to the human reader. The context already indicates it is a TPDU state and hence TPDU_S_ prefix doesn't add any non-redundant information https://gerrit.osmocom.org/#/c/11361/1/firmware/libcommon/source/card_emu.c@671 PS1, Line 671: TRACE_DEBUG("%u: 7816 TPDU state %u (%s) -> %u (%s)\r\n", ch->num, printing both the number and the string makes little sense, IMHO. In other places [of osmocom projects] we normally print only either the number or the string representation, not both. Please note that get_value_string will generate an "unknown 0x1234" string if the value doesn't have a corresponding string. The latter unfortunaly doesn't work if you call get_value_string() twice from function arguments, as the returned buffer is a static buffer. You can leave it as-is, but I'm juts sharing my thoughts. None of them are critical or prevent this from being merged. -- To view, visit https://gerrit.osmocom.org/11361 To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings Gerrit-Project: simtrace2 Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id5104a2c3579dedb092c179748e9ed525673841c Gerrit-Change-Number: 11361 Gerrit-PatchSet: 1 Gerrit-Owner: Kévin Redon <kre...@sysmocom.de> Gerrit-Reviewer: Harald Welte <lafo...@gnumonks.org> Gerrit-Reviewer: Jenkins Builder (1000002) Gerrit-Comment-Date: Mon, 15 Oct 2018 22:57:49 +0000 Gerrit-HasComments: Yes Gerrit-HasLabels: Yes
