[{"id":15665,"web_url":"https://patchwork.libcamera.org/comment/15665/","msgid":"","date":"2021-03-14T01:40:46","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add a\n\ttoString()","submitter":{"id":2,"url":"https://patchwork.libcamera.org/api/people/2/","name":"Laurent Pinchart","email":"laurent.pinchart@ideasonboard.com"},"content":"Hi Kieran,\n\nThank you for the patch.\n\nOn Fri, Mar 12, 2021 at 06:11:25AM +0000, Kieran Bingham wrote:\n> Provide a toString helper to assist in printing Request state\n> for debug and logging contexts.\n> \n> Signed-off-by: Kieran Bingham \n> ---\n> include/libcamera/request.h | 2 ++\n> src/libcamera/request.cpp | 20 +++++++++++++++++++-\n> 2 files changed, 21 insertions(+), 1 deletion(-)\n> \n> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n> index 6f2f881e840a..59d7f4bac0d2 100644\n> --- a/include/libcamera/request.h\n> +++ b/include/libcamera/request.h\n> @@ -56,6 +56,8 @@ public:\n> \n> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n> \n> +\tconst std::string toString() const;\n\nYou can drop the initial const.\n\nAnd you should include .\n\n> +\n> private:\n> \tLIBCAMERA_DISABLE_COPY(Request)\n> \n> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n> index 24c3694de5fc..12c2e7d425f9 100644\n> --- a/src/libcamera/request.cpp\n> +++ b/src/libcamera/request.cpp\n> @@ -268,7 +268,7 @@ void Request::complete()\n> \tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n> \n> \tLOG(Request, Debug)\n> -\t\t<< \"Request has completed - cookie: \" << cookie_\n> +\t\t<< toString() << \" has completed - cookie: \" << cookie_\n> \t\t<< (cancelled_ ? \" [Cancelled]\" : \"\");\n\nInheriting from Loggable would be nicer, but that's not part of the\npublic API :-S\n\nShould the cookie and cancelled state be handled in toString() ?\n\n> \n> \tLIBCAMERA_TRACEPOINT(request_complete, this);\n> @@ -302,4 +302,22 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n> \treturn !hasPendingBuffers();\n> }\n> \n> +const std::string Request::toString() const\n> +{\n> +\tstd::stringstream ss;\n> +\n> +\tstatic const char *statuses[] = {\n> +\t\t\"Pending\",\n> +\t\t\"Complete\",\n> +\t\t\"Cancelled\",\n> +\t};\n> +\n> +\tss << \"Request (\" << sequence_ << \") \" << statuses[status_];\n> +\n> +\tif (hasPendingBuffers())\n> +\t\tss << \" [Pending:\" << pending_.size() << \"]\";\n\nShould we make this a bit more concise ? It can then become a bit\ncryptic, but as it's a debugging tool, I'd focus on making it readable\nfor the trained eye (as well as possibly easily machine-parseable).\nHaving abbreviated status names, for instance, to give them all the same\nlength, may not look as nice at first, but when you read thousands of\nlines of log, it quickly makes it easier I think.\n\nI'm thinking about something like this:\n\n\tstatic const char *statuses = \"PCX\";\n\n\t\"Request(%u:%c:%u)\", sequence_, statuses[status_], pending_.size()\n\n(it's the most extreme case, a middle ground is certainly possible with\nlonger statuses, and possibly even the full status name)\n\nMaybe this is just me ?\n\n> +\n> +\treturn ss.str();\n> +}\n> +\n> } /* namespace libcamera */","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id E0D7EBD1F1\n\tfor ;\n\tSun, 14 Mar 2021 01:41:24 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id 4A7DA60850;\n\tSun, 14 Mar 2021 02:41:24 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 345B46084D\n\tfor ;\n\tSun, 14 Mar 2021 02:41:23 +0100 (CET)","from pendragon.ideasonboard.com (62-78-145-57.bb.dnainternet.fi\n\t[62.78.145.57])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id 921BC55C;\n\tSun, 14 Mar 2021 02:41:22 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"g3mn8lX4\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615686082;\n\tbh=2M3u4jSDdZCynVTQ+wKqaXm73uWw60Fa5q3n/4Z7Kps=;\n\th=Date:From:To:Cc:Subject:References:In-Reply-To:From;\n\tb=g3mn8lX45TWVceahJLRym92QyZigltK9HkJvx3mxilpBRyf4+cWSHhpeH9N5cxZih\n\tvl5Kx6qgjGgFNHEFV4KN6qHS/ChWaKZsCJPd8aWu4YqdC36Bb+NIVWc3TN6fyvoc7Q\n\tl3TdfOcz0p4xMAZWhbh0ZsAFp6Kp1Nvnz5CBPzho=","Date":"Sun, 14 Mar 2021 03:40:46 +0200","From":"Laurent Pinchart ","To":"Kieran Bingham ","Message-ID":"","References":"<20210312061131.854849-1-kieran.bingham@ideasonboard.com>\n\t<20210312061131.854849-3-kieran.bingham@ideasonboard.com>","MIME-Version":"1.0","Content-Disposition":"inline","In-Reply-To":"<20210312061131.854849-3-kieran.bingham@ideasonboard.com>","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add a\n\ttoString()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}},{"id":15698,"web_url":"https://patchwork.libcamera.org/comment/15698/","msgid":"<76265824-81e6-3ced-502f-a47523fcb88e@ideasonboard.com>","date":"2021-03-15T12:04:34","subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add a\n\ttoString()","submitter":{"id":4,"url":"https://patchwork.libcamera.org/api/people/4/","name":"Kieran Bingham","email":"kieran.bingham@ideasonboard.com"},"content":"Hi Laurent,\n\nOn 14/03/2021 01:40, Laurent Pinchart wrote:\n> Hi Kieran,\n> \n> Thank you for the patch.\n> \n> On Fri, Mar 12, 2021 at 06:11:25AM +0000, Kieran Bingham wrote:\n>> Provide a toString helper to assist in printing Request state\n>> for debug and logging contexts.\n>>\n>> Signed-off-by: Kieran Bingham \n>> ---\n>> include/libcamera/request.h | 2 ++\n>> src/libcamera/request.cpp | 20 +++++++++++++++++++-\n>> 2 files changed, 21 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/include/libcamera/request.h b/include/libcamera/request.h\n>> index 6f2f881e840a..59d7f4bac0d2 100644\n>> --- a/include/libcamera/request.h\n>> +++ b/include/libcamera/request.h\n>> @@ -56,6 +56,8 @@ public:\n>> \n>> \tbool hasPendingBuffers() const { return !pending_.empty(); }\n>> \n>> +\tconst std::string toString() const;\n> \n> You can drop the initial const.\n> \n> And you should include .\n\nAck,\n\n> \n>> +\n>> private:\n>> \tLIBCAMERA_DISABLE_COPY(Request)\n>> \n>> diff --git a/src/libcamera/request.cpp b/src/libcamera/request.cpp\n>> index 24c3694de5fc..12c2e7d425f9 100644\n>> --- a/src/libcamera/request.cpp\n>> +++ b/src/libcamera/request.cpp\n>> @@ -268,7 +268,7 @@ void Request::complete()\n>> \tstatus_ = cancelled_ ? RequestCancelled : RequestComplete;\n>> \n>> \tLOG(Request, Debug)\n>> -\t\t<< \"Request has completed - cookie: \" << cookie_\n>> +\t\t<< toString() << \" has completed - cookie: \" << cookie_\n>> \t\t<< (cancelled_ ? \" [Cancelled]\" : \"\");\n> \n> Inheriting from Loggable would be nicer, but that's not part of the\n> public API :-S\n>\n\nYes, been here ;-)\n\n> Should the cookie and cancelled state be handled in toString() ?\n\nCancelled is already handled because the state gets printed.\nAlthough - indeed - before this call if cancelled was set - that\nwouldn't have been updated yet.\n\nI guess I was only adding the information I cared about (the sequence,\nand state) ... the cookie is application specific - so I don't think we\ncare about that when dealing with internal debug ...\n\nAnd I don't yet know how much cross over there will be between tracking\ninternal debug against whatever the applciation stores in the cookie.\n\n\n>> \n>> \tLIBCAMERA_TRACEPOINT(request_complete, this);\n>> @@ -302,4 +302,22 @@ bool Request::completeBuffer(FrameBuffer *buffer)\n>> \treturn !hasPendingBuffers();\n>> }\n>> \n>> +const std::string Request::toString() const\n>> +{\n>> +\tstd::stringstream ss;\n>> +\n>> +\tstatic const char *statuses[] = {\n>> +\t\t\"Pending\",\n>> +\t\t\"Complete\",\n>> +\t\t\"Cancelled\",\n>> +\t};\n>> +\n>> +\tss << \"Request (\" << sequence_ << \") \" << statuses[status_];\n>> +\n>> +\tif (hasPendingBuffers())\n>> +\t\tss << \" [Pending:\" << pending_.size() << \"]\";\n> \n> Should we make this a bit more concise ? It can then become a bit\n> cryptic, but as it's a debugging tool, I'd focus on making it readable\n> for the trained eye (as well as possibly easily machine-parseable).\n> Having abbreviated status names, for instance, to give them all the same\n> length, may not look as nice at first, but when you read thousands of\n> lines of log, it quickly makes it easier I think.\n> \n> I'm thinking about something like this:\n> \n> \tstatic const char *statuses = \"PCX\";\n> \n> \t\"Request(%u:%c:%u)\", sequence_, statuses[status_], pending_.size()\n> \n> (it's the most extreme case, a middle ground is certainly possible with\n> longer statuses, and possibly even the full status name)\n> \n> Maybe this is just me ?\n\nHrm ... I don't know yet. I 'liked' having the extended print while\ntrawling through the logs - but it could be made more concise.\n\nYou're example isn't so bad, and I like that it would then be machine\nparseable too.\n\nI'll try it out.\n\n\n> \n>> +\n>> +\treturn ss.str();\n>> +}\n>> +\n>> } /* namespace libcamera */\n>","headers":{"Return-Path":"","X-Original-To":"parsemail@patchwork.libcamera.org","Delivered-To":"parsemail@patchwork.libcamera.org","Received":["from lancelot.ideasonboard.com (lancelot.ideasonboard.com\n\t[92.243.16.209])\n\tby patchwork.libcamera.org (Postfix) with ESMTPS id 42B37BD80C\n\tfor ;\n\tMon, 15 Mar 2021 12:04:40 +0000 (UTC)","from lancelot.ideasonboard.com (localhost [IPv6:::1])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTP id ADDD868D40;\n\tMon, 15 Mar 2021 13:04:39 +0100 (CET)","from perceval.ideasonboard.com (perceval.ideasonboard.com\n\t[213.167.242.64])\n\tby lancelot.ideasonboard.com (Postfix) with ESMTPS id 4269F60106\n\tfor ;\n\tMon, 15 Mar 2021 13:04:37 +0100 (CET)","from [192.168.0.20]\n\t(cpc89244-aztw30-2-0-cust3082.18-1.cable.virginm.net [86.31.172.11])\n\tby perceval.ideasonboard.com (Postfix) with ESMTPSA id B199D556;\n\tMon, 15 Mar 2021 13:04:36 +0100 (CET)"],"Authentication-Results":"lancelot.ideasonboard.com;\n\tdkim=fail reason=\"signature verification failed\" (1024-bit key;\n\tunprotected) header.d=ideasonboard.com header.i=@ideasonboard.com\n\theader.b=\"k4xUaXlY\"; dkim-atps=neutral","DKIM-Signature":"v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com;\n\ts=mail; t=1615809876;\n\tbh=P7EznpJjMqndvXOS86zW68O9F5QGFOe8bGQgLIn1yVk=;\n\th=Reply-To:Subject:To:Cc:References:From:Date:In-Reply-To:From;\n\tb=k4xUaXlYekMzfkABx7WSZAR7wdwxm9THWn/RQqIYtcrIe+urw7WP8RoogD/qRQwDS\n\t7CZxgYeV7Sdo+qNa/cBtF5yIQrpg1EZi21CsJC3R/1BLUWKUzfQgujTWeNhVh0ktI+\n\tD0UmzrXhRKvvkLPSB8WyIc1pVvnmas4tGbdIZzKE=","To":"Laurent Pinchart ","References":"<20210312061131.854849-1-kieran.bingham@ideasonboard.com>\n\t<20210312061131.854849-3-kieran.bingham@ideasonboard.com>\n\t","From":"Kieran Bingham ","Autocrypt":"addr=kieran.bingham@ideasonboard.com; keydata=\n\tmQINBFYE/WYBEACs1PwjMD9rgCu1hlIiUA1AXR4rv2v+BCLUq//vrX5S5bjzxKAryRf0uHat\n\tV/zwz6hiDrZuHUACDB7X8OaQcwhLaVlq6byfoBr25+hbZG7G3+5EUl9cQ7dQEdvNj6V6y/SC\n\trRanWfelwQThCHckbobWiQJfK9n7rYNcPMq9B8e9F020LFH7Kj6YmO95ewJGgLm+idg1Kb3C\n\tpotzWkXc1xmPzcQ1fvQMOfMwdS+4SNw4rY9f07Xb2K99rjMwZVDgESKIzhsDB5GY465sCsiQ\n\tcSAZRxqE49RTBq2+EQsbrQpIc8XiffAB8qexh5/QPzCmR4kJgCGeHIXBtgRj+nIkCJPZvZtf\n\tKr2EAbc6tgg6DkAEHJb+1okosV09+0+TXywYvtEop/WUOWQ+zo+Y/OBd+8Ptgt1pDRyOBzL8\n\tRXa8ZqRf0Mwg75D+dKntZeJHzPRJyrlfQokngAAs4PaFt6UfS+ypMAF37T6CeDArQC41V3ko\n\tlPn1yMsVD0p+6i3DPvA/GPIksDC4owjnzVX9kM8Zc5Cx+XoAN0w5Eqo4t6qEVbuettxx55gq\n\t8K8FieAjgjMSxngo/HST8TpFeqI5nVeq0/lqtBRQKumuIqDg+Bkr4L1V/PSB6XgQcOdhtd36\n\tOe9X9dXB8YSNt7VjOcO7BTmFn/Z8r92mSAfHXpb07YJWJosQOQARAQABtDBLaWVyYW4gQmlu\n\tZ2hhbSA8a2llcmFuLmJpbmdoYW1AaWRlYXNvbmJvYXJkLmNvbT6JAlcEEwEKAEECGwMFCwkI\n\tBwIGFQgJCgsCBBYCAwECHgECF4ACGQEWIQSQLdeYP70o/eNy1HqhHkZyEKRh/QUCXWTtygUJ\n\tCyJXZAAKCRChHkZyEKRh/f8dEACTDsbLN2nioNZMwyLuQRUAFcXNolDX48xcUXsWS2QjxaPm\n\tVsJx8Uy8aYkS85mdPBh0C83OovQR/OVbr8AxhGvYqBs3nQvbWuTl/+4od7DfK2VZOoKBAu5S\n\tQK2FYuUcikDqYcFWJ8DQnubxfE8dvzojHEkXw0sA4igINHDDFX3HJGZtLio+WpEFQtCbfTAG\n\tYZslasz1YZRbwEdSsmO3/kqy5eMnczlm8a21A3fKUo3g8oAZEFM+f4DUNzqIltg31OAB/kZS\n\tenKZQ/SWC8PmLg/ZXBrReYakxXtkP6w3FwMlzOlhGxqhIRNiAJfXJBaRhuUWzPOpEDE9q5YJ\n\tBmqQL2WJm1VSNNVxbXJHpaWMH1sA2R00vmvRrPXGwyIO0IPYeUYQa3gsy6k+En/aMQJd27dp\n\taScf9am9PFICPY5T4ppneeJLif2lyLojo0mcHOV+uyrds9XkLpp14GfTkeKPdPMrLLTsHRfH\n\tfA4I4OBpRrEPiGIZB/0im98MkGY/Mu6qxeZmYLCcgD6qz4idOvfgVOrNh+aA8HzIVR+RMW8H\n\tQGBN9f0E3kfwxuhl3omo6V7lDw8XOdmuWZNC9zPq1UfryVHANYbLGz9KJ4Aw6M+OgBC2JpkD\n\thXMdHUkC+d20dwXrwHTlrJi1YNp6rBc+xald3wsUPOZ5z8moTHUX/uPA/qhGsbkCDQRWBP1m\n\tARAAzijkb+Sau4hAncr1JjOY+KyFEdUNxRy+hqTJdJfaYihxyaj0Ee0P0zEi35CbE6lgU0Uz\n\ttih9fiUbSV3wfsWqg1Ut3/5rTKu7kLFp15kF7eqvV4uezXRD3Qu4yjv/rMmEJbbD4cTvGCYI\n\td6MDC417f7vK3hCbCVIZSp3GXxyC1LU+UQr3fFcOyCwmP9vDUR9JV0BSqHHxRDdpUXE26Dk6\n\tmhf0V1YkspE5St814ETXpEus2urZE5yJIUROlWPIL+hm3NEWfAP06vsQUyLvr/GtbOT79vXl\n\tEn1aulcYyu20dRRxhkQ6iILaURcxIAVJJKPi8dsoMnS8pB0QW12AHWuirPF0g6DiuUfPmrA5\n\tPKe56IGlpkjc8cO51lIxHkWTpCMWigRdPDexKX+Sb+W9QWK/0JjIc4t3KBaiG8O4yRX8ml2R\n\t+rxfAVKM6V769P/hWoRGdgUMgYHFpHGSgEt80OKK5HeUPy2cngDUXzwrqiM5Sz6Od0qw5pCk\n\tNlXqI0W/who0iSVM+8+RmyY0OEkxEcci7rRLsGnM15B5PjLJjh1f2ULYkv8s4SnDwMZ/kE04\n\t/UqCMK/KnX8pwXEMCjz0h6qWNpGwJ0/tYIgQJZh6bqkvBrDogAvuhf60Sogw+mH8b+PBlx1L\n\toeTK396wc+4c3BfiC6pNtUS5GpsPMMjYMk7kVvEAEQEAAYkCPAQYAQoAJgIbDBYhBJAt15g/\n\tvSj943LUeqEeRnIQpGH9BQJdizzIBQkLSKZiAAoJEKEeRnIQpGH9eYgQAJpjaWNgqNOnMTmD\n\tMJggbwjIotypzIXfhHNCeTkG7+qCDlSaBPclcPGYrTwCt0YWPU2TgGgJrVhYT20ierN8LUvj\n\t6qOPTd+Uk7NFzL65qkh80ZKNBFddx1AabQpSVQKbdcLb8OFs85kuSvFdgqZwgxA1vl4TFhNz\n\tPZ79NAmXLackAx3sOVFhk4WQaKRshCB7cSl+RIng5S/ThOBlwNlcKG7j7W2MC06BlTbdEkUp\n\tECzuuRBv8wX4OQl+hbWbB/VKIx5HKlLu1eypen/5lNVzSqMMIYkkZcjV2SWQyUGxSwq0O/sx\n\tS0A8/atCHUXOboUsn54qdxrVDaK+6jIAuo8JiRWctP16KjzUM7MO0/+4zllM8EY57rXrj48j\n\tsbEYX0YQnzaj+jO6kJtoZsIaYR7rMMq9aUAjyiaEZpmP1qF/2sYenDx0Fg2BSlLvLvXM0vU8\n\tpQk3kgDu7kb/7PRYrZvBsr21EIQoIjXbZxDz/o7z95frkP71EaICttZ6k9q5oxxA5WC6sTXc\n\tMW8zs8avFNuA9VpXt0YupJd2ijtZy2mpZNG02fFVXhIn4G807G7+9mhuC4XG5rKlBBUXTvPU\n\tAfYnB4JBDLmLzBFavQfvonSfbitgXwCG3vS+9HEwAjU30Bar1PEOmIbiAoMzuKeRm2LVpmq4\n\tWZw01QYHU/GUV/zHJSFk","Organization":"Ideas on Board","Message-ID":"<76265824-81e6-3ced-502f-a47523fcb88e@ideasonboard.com>","Date":"Mon, 15 Mar 2021 12:04:34 +0000","User-Agent":"Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101\n\tThunderbird/68.10.0","MIME-Version":"1.0","In-Reply-To":"","Content-Language":"en-GB","Subject":"Re: [libcamera-devel] [PATCH 2/8] libcamera: request: Add a\n\ttoString()","X-BeenThere":"libcamera-devel@lists.libcamera.org","X-Mailman-Version":"2.1.29","Precedence":"list","List-Id":"","List-Unsubscribe":",\n\t","List-Archive":"","List-Post":"","List-Help":"","List-Subscribe":",\n\t","Reply-To":"kieran.bingham@ideasonboard.com","Cc":"libcamera devel ","Content-Type":"text/plain; charset=\"us-ascii\"","Content-Transfer-Encoding":"7bit","Errors-To":"libcamera-devel-bounces@lists.libcamera.org","Sender":"\"libcamera-devel\" "}}]