<p style="white-space: pre-wrap; word-wrap: break-word;">I'm sorry to criticise as much as this. I started out wanting to just +2 straight away, but ended up finding numerous trivial tweaks that would greatly serve readability. The bottom line is, this is probably correct, but I am distracted by the writing style ;)</p><p>Patch set 2:<span style="border-radius: 3px; display: inline-block; margin: 0 2px; padding: 4px;background-color: #ffd4d4;">Code-Review -1</span></p><p><a href="https://gerrit.osmocom.org/11836">View Change</a></p><p>12 comments:</p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0;"><p><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc">File common/chapters/gsup.adoc:</a></p><ul style="list-style: none; padding: 0;"><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@507">Patch Set #2, Line 507:</a> <code style="font-family:monospace,monospace">==== MO-forwardSM Request</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm slightly critical of the mix of dash and non-caps / CamelCase / acronym style. Why not just "MO Forward Short Message Request" -- that is easy to understand for the reader on first sight.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@509">Patch Set #2, Line 509:</a> <code style="font-family:monospace,monospace">Direction: SGSN / MSC => ESME (through VLR and HLR)</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">I'm curious why you name the SGSN first. So far I wasn't aware of our SGSN even able to send SMS.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@522">Patch Set #2, Line 522:</a> <code style="font-family:monospace,monospace">This message is used to forward _MO (Mobile Originated)_ short messages</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">would be nice to first say it in simple rather nontechnical terms and then add details. Like, "When a subscriber sends an SMS, this message forwards it to the ESME. [...]"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@524">Patch Set #2, Line 524:</a> <code style="font-family:monospace,monospace">Entity). The corresponding MAP service (defined in 3GPP TS 29.002) is</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">We have a glossary of acronyms, maybe rather make sure they are in the glossary and abbreviate everywhere / choose to write them out everywhere. Naming both makes it harder to follow the sentence flow. Same goes for indicating "MS (CS domain) / SGSN (PS domain)" all the time, this is not the place to explain what PS and CS mean, so rather assume the reader knows these acronyms.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@525">Patch Set #2, Line 525:</a> <code style="font-family:monospace,monospace">*MAP-MO-FORWARD-SHORT-MESSAGE*, described in section 12.2.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">rather give the spec reference once, complete, in the end "... (see 3GPP TS 29.002 section 12.2)"</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@528">Patch Set #2, Line 528:</a> <code style="font-family:monospace,monospace">attempt and _RP-SMMA_ notification attempt on the _SM-RL (Relay Layer)_.</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">"attempt" sounds like it fails. "request"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@532">Patch Set #2, Line 532:</a> <code style="font-family:monospace,monospace">Direction: ESME (through VLR and HLR) => SGSN / MSC</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">above it spells  "MSC => ESME (through VLR and HLR)" -- maybe rather reverse it here? "ESME (through HLR and VLR) -> MSC"?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@545">Patch Set #2, Line 545:</a> <code style="font-family:monospace,monospace">short message delivery from SGSN (PS domain) / MSC (CS domain) to an ESME</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">above it says ESME => MSC, in this sentence the direction is reversed? Copy-paste error? Or did you mean that the earlier message was in the other direction? To avoid the confusion, you could simply not name the direction again, just rely on the reader looking at the direction definition on top and knowing that an error response is for an earlier request in the other direction.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@546">Patch Set #2, Line 546:</a> <code style="font-family:monospace,monospace">(External Short Message Entity). Thus it triggers an _RP-ERROR_ on the _SM-RL</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">this is not the place to define what should happen in the implementation, is it? If at all, this could indicate "should usually trigger", but foremost, this explains the message definition, and the semantics are up to the implementation / explained elsewhere.</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@1094">Patch Set #2, Line 1094:</a> <code style="font-family:monospace,monospace">Relay Layer)_ has an unique _message reference (see 8.2.3)_, that</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">Don't ask me why, but it is "a unique". I know the consonant vs vocal rule, but that is not as deterministic as one might think. There is also "an RSL procedure" because "R" is spoken "arr"...</p><p style="white-space: pre-wrap; word-wrap: break-word;">8.2.3 in this document? then use a <<foo>> link. 8.2.3 in 04.11? then write the full '3GPP TS 04.11 section 8.2.3' out here, but rather not break the flow of the sentence with it.</p><p style="white-space: pre-wrap; word-wrap: break-word;">I'd not use italics at all. If they are "given names", capitalize. If already capitalized, italics are redundant. If you really must use italics, don't also use italics for '(see 8.2.3)'. (I used to use a lot of markup too in the past, but now my opinion is they are too much effort and rather than helping, they clutter up the reading flow.)</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@1110">Patch Set #2, Line 1110:</a> <code style="font-family:monospace,monospace">* IMSI (see clause 7.6.2.1);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">clause 7.6.2.1 *where*?</p></li><li style="margin: 0; padding: 0 0 0 16px;"><p style="margin-bottom: 4px;"><a href="https://gerrit.osmocom.org/#/c/11836/2/common/chapters/gsup.adoc@1111">Patch Set #2, Line 1111:</a> <code style="font-family:monospace,monospace">* LMSI (see clause 7.6.2.16, not implemented!);</code></p><p style="white-space: pre-wrap; word-wrap: break-word;">clarify "not implemented" -- this here should be a protocol definition: it is by definition implemented or not implemented by the *implementations* (libosmo-gsup-foo), this protocol definition cannot be "not implemented" by itself.</p></li></ul></li></ul><p>To view, visit <a href="https://gerrit.osmocom.org/11836">change 11836</a>. To unsubscribe, or for help writing mail filters, visit <a href="https://gerrit.osmocom.org/settings">settings</a>.</p><div itemscope itemtype="http://schema.org/EmailMessage"><div itemscope itemprop="action" itemtype="http://schema.org/ViewAction"><link itemprop="url" href="https://gerrit.osmocom.org/11836"/><meta itemprop="name" content="View Change"/></div></div>

<div style="display:none"> Gerrit-Project: osmo-gsm-manuals </div>
<div style="display:none"> Gerrit-Branch: master </div>
<div style="display:none"> Gerrit-MessageType: comment </div>
<div style="display:none"> Gerrit-Change-Id: Ie0150756c33c1352bc4eb49421824542c711175c </div>
<div style="display:none"> Gerrit-Change-Number: 11836 </div>
<div style="display:none"> Gerrit-PatchSet: 2 </div>
<div style="display:none"> Gerrit-Owner: Vadim Yanitskiy <axilirator@gmail.com> </div>
<div style="display:none"> Gerrit-Reviewer: Harald Welte <laforge@gnumonks.org> </div>
<div style="display:none"> Gerrit-Reviewer: Jenkins Builder (1000002) </div>
<div style="display:none"> Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr@sysmocom.de> </div>
<div style="display:none"> Gerrit-Comment-Date: Mon, 19 Nov 2018 23:25:12 +0000 </div>
<div style="display:none"> Gerrit-HasComments: Yes </div>
<div style="display:none"> Gerrit-HasLabels: Yes </div>