Change in simtrace2[master]: cardem: add more debug information for TPDU state

[Harald Welte]

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 <kredon at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Comment-Date: Mon, 15 Oct 2018 22:57:49 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20181015/9a695f0c/attachment.htm>