[ipcdn] RE: MIB Doctor review: http://www.ietf.org/internet-drafts/draft-ietf-ipcdn-pktc-signaling-13.txt

Bert,

Comments inline.

> Let me start to tell you that I am not a voice expert, so a lot of the

> actual content of this MIB module is abacadabra for me. I am assuming 
> that other IPCDN WG members have evaluated (or will
> evaluate) the actual content w.r.t. the technical details and 
> correctness.
> 
> Let me also say that I find this MIB module pretty wieldy,

Thanks for the review and this opening comment. It is nice to hear and
the credit goes to all the ipcdn participants & implementers who
contributed to, and revised, the mib module over the years.

> More or less serious issues/concerns:
> 
> - writable objects MUST specify (for example in their DESCRIPTION
>   clause) the expected persistence behavior.

Good point. The following MIB Objects are writable:

pktcSigDevCidSigProtocol, pktcSigDevR0Cadence, pktcSigDevR1Cadence,
pktcSigDevR2Cadence, pktcSigDevR3Cadence, pktcSigDevR4Cadence,
pktcSigDevR5Cadence, pktcSigDevR6Cadence, pktcSigDevR7Cadence,
pktcSigDevRgCadence, pktcSigDefCallSigDscp, pktcSigDefMediaStreamDscp,
pktcSigPulseSignalFrequency, pktcSigPulseSignalDbLevel,
pktcSigPulseSignalDuration, pktcSigPulseSignalPulseInterval,
pktcSigPulseSignalRepeatCount, pktcSigDevCidMode,
pktcSigDevCidAfterRing, pktcSigDevCidAfterDTAS, pktcSigDevCidAfterRPAS,
pktcSigDevRingAfterCID, pktcSigDevCidDTASAfterLR, pktcSigDevVmwiMode,
pktcSigDevVmwiAfterDTAS, pktcSigDevVmwiAfterRPAS,
pktcSigDevVmwiDTASAfterLR, pktcSigDevRingCadence,
pktcSigDevCidDelayAfterLR, pktcSigDevCidDtmfStartCode,
pktcSigDevCidDtmfEndCode, pktcSigDevVmwiSigProtocol,
pktcSigDevVmwiDelayAfterLR, pktcSigDevVmwiDtmfStartCode,
pktcSigDevVmwiDtmfEndCode, pktcSigDevrpAsDtsDuration.

We did not intend to persist any of these MIB Objects. The DESCRIPTION
clause will be updated to reflect this.

> - I see various objects aka:
>    pktcSigDevR0Cadence     OBJECT-TYPE
>        SYNTAX      PktcRingCadence
>        MAX-ACCESS  read-write
>        STATUS      current
>        DESCRIPTION
>            " This object specifies ring cadence 0 (a user defined
>              field). This object is required for the L line package."
>        ::= { pktcSigDevObjects 5 }
> 
>   The fact that an object is required" is normally expressed in the
>   MODULE-COMPLIANCE. The above object (together with several others)
>   seems conditionaly optional, which means you would group all such
>   objects required for L line package in one OBJECT-GROUP and make
>   that OBJECT-GROUP conditionaly mandatory.

Your proposal makes sense. I propose we add a new object-group and
indicate it to be conditionally mandatory as you suggest.

> - I wonder if the SYNTAX of SnmpAdminString makes sense for the
>   objects pktcSigCapabilityVersion and pktcSigCapabilityVendorExt.
>   It will work. But, SnmpAdminString is intended to contain human
>   readable (in any language/character set) strings. It seems that the
>   values that you allow are very restricted and certainly cannot
>   be in any other language/character-set.
>   I personally can live with it... but you might want to
>   think of just an OCTET-STRING that you define exactly what it can
>   contain.

Can see your point; however, given the original intention to not be
restrictive regarding the values and to restrict this to human readable
strings only, we should probably let this be as-is. 

> - I find this strange:
>   pktcSigPowerRingFrequency    OBJECT-TYPE
>       SYNTAX       INTEGER {
>                    f20Hz(1),
>                    f25Hz(2),
>                    f33Point33Hz(3),
>                    f50Hz(4),
>                    f15Hz(5),
>                    f16Hz(6),
>                    f22Hz(7),
>                    f23Hz(8),
>                    f45Hz(9)
>       }
>       UNITS "Hertz"
> 
>   Because tha value of the object is certainly not in "Hertz" units.
>   A value of 1 doe not represent 1 UNIT of "Hertz", but it represents
>   20 Hertz. and so on... I think I would remove the UNITS clause.

Yes, good catch, UNITS clause will be removed.

>   Same for pktcSigPulseSignalFrequency

Yes, UNITS clause will be removed.

> - pktcSigPulseSignalTable DESCRIPTION clause speaks about the 
> mandatory
>   nature of this table for E line package. This is MODULE-COMPLIANCE
>   stuff and should be expressed in the OBJECT-GROUP grouping and
>   MODULE-COMPLIANCE.
> 
>   similar comment for pktcSigDevRingCadenceTable
I propose we add another new object-group and make it conditionally
mandatory as you suggested earlier.

>
> - I see:
>   pktcSigPulseSignalDbLevel    OBJECT-TYPE
>       SYNTAX       TenthdBm (-350..0)
>       UNITS        "dBm"
> 
>   I thought that the units were "1/10 of a dBm" ??
> 
>   same for pktcSigDevToneDbLevel

Yes, will be updated.

> - Given the DESCRIPTION clause for
>    pktcSigPulseSignalRepeatCount    OBJECT-TYPE
>        SYNTAX       Unsigned32
>   You might consider to make the syntax
>        SYNTAX       Unsigned32 (1..50)

Yes, will be updated.

> - This DESCRIPTION clause has to be incorrect:
> 
>    pktcSigDevRingCadenceEntry    OBJECT-TYPE
>        SYNTAX       PktcSigDevRingCadenceEntry
>        MAX-ACCESS   not-accessible
>        STATUS       current
>        DESCRIPTION
>            " Unique value ranging from 0 to 127 that will correspond 
> to
>              the different ring cadences that are being supported by
>              the device."
> 
>   After all, it is an "entry" or "row" definition and so it does not 
> have a
>   value from 0-127. The INDEX object does.
>   The DESCRIPTION clause of the INDEX object seems to be describing 
> what an
>   Entry is all about.

Good catch. Will be corrected.

> - What sort of duration (UNITS?) must I assume for:
>       pktcSigDevToneFreqOnDuration OBJECT-TYPE
>           SYNTAX       Unsigned32(0..5000)
> 
>   same question for pktcSigDevToneFreqOffDuration,

According to the clarification provided by Satish Kumar and Phil
Freyman, this is in milliseconds

> - Have seen a SYNTAX of
>           SYNTAX       INTEGER {
>                                fsk(1),
>                                dtmf(2)
>           }
>   for the signaling protocol multiple times.
>   Candidate for a TEXTUAL-CONVENTION?

Yes, a TC will be created.

> - For pktcNcsEndPntConfigPartialDialTO, what does a zero value mean?
> 
>   similar question for the other pktcNcs....TO objects

I think the DESCRIPTION needs some additional explanation. To better
respond to your question, here is the response from Phil Freyman who
articulates the reasoning...

"Logically the TO objects will be longer than any tone or frequency
duration that it applies to since it is really a time out max for "bad"
signaling situations. Per other text the TO object takes priority over
any duration or cadence settings so if the TO object is set to "0"
seconds, then the related tone would not be played out (timed out).
Regardless of any duration or cadence setting. We cannot state that the
TO must be longer than the associated tone duration since some durations
are continuous and therefore the value of a time out would be lost...."

> - I find it strange to see pktcNcsEndPntConfigStatus in the middle of 
> the
>   table. But... it is not an error.
OK, I suggest we leave it as-is at this point.

> - For the read-create table, I wonder where the read-only objects
>   pktcNcsEndPntStatusCallIpAddressType and 
> pktcNcsEndPntStatusCallIpAddress
>   come from? How does the agent determine those addresses.?

The DESCRIPTION needs to clarify this. To explain further, the agent
determines the CMS FQDN from the MIB Object
'pktcNcsEndPntConfigCallAgentId'. It then uses DNS to resolve the IP
address. This resolution can lead to multiple IP addresses and it picks
one. It then populates 'pktcNcsEndPntStatusCallIpAddress' with this IP
address.

> - For pktcNcsGroup the DESCRIPTION clause talks about this group being
>   mandatory for... That does NOT belong here. The fact if a group is
>   mandatory or not is specified in the MODULE-COMPLIANCE statement.

Yes, this will be fixed.

> Admin/Naming questions:
> 
> - The title speaks about:
> 
>       Network-Based Call Signaling (NCS) MIB for PacketCable and
> 
>   while the MIB Module is named: PKTC-IETF-SIG-MIB and pktcIetfSigMib
>   Not that that is a bug... but it feels somewhat strange.
> 
>   Later in the document, at various places the "NCS MIB" term comes 
> back,
>   and so people might expect to see IETF-NCS-MIB or ietfNcsMib as 
> names?

Let me check with the co-authors. I would leave it as-is, but I think we
need to clean up the text accordingly.

> 
> - Section 4 states:
> 
>    Terminal Adapter (MTA) devices. The IETF NCS MIB module (PKTC-IETF-
>    SIG-MIB) is intended to supersede various Signaling MIB modules 
> from
>    which it is partly derived:
>      - the PacketCable 1.0 Signaling MIB Specification
>        [PKT-SP-MIB-SIG-1.0],
>      - the PacketCable 1.5 Signaling MIB Specification
>        [PKT-SP-MIB-SIG-1.5],
>      - the ITU-T IPCablecom Signaling MIB requirements [ITU-T-J169],
>      - the ETSI Signaling MIB [ETSI-TS-101-909-9]. The ETSI Signaling
>        MIB requirements also refer to various signal characteristics
>        defined in [ETSI-TS-101-909-4], [ETSI-EN-300-001],
>        [ETSI-EN-300-659-1], [ETSI-EN-300-324-1] and [ETSI-TR-101-183].
> 
>   I know that many IPCDN WG members are all participating in 
> PackagetCable,
>   so I assume that superseding (is that same as obsoleting in IETF
> terms?)
>   PacketCable documents is fine. But how about ITU-T and ETSI? Are 
> they
>   OK with the above statements?

Good point, unless we formally receive a liaison statement about this,
we should be more careful. Let's replace "intended to supersede" with
"intended to update" which gives these 2 SDOs more room & control to do
what they think is right.

> - In section 4.1 I would use "MIB module" instead of "MIB"

Ok, will be fixed.

> 
> - The groups listed in section 4.1 are not the same as the groups
>   defined with the OBJECT-GROUP macros.

Ok, will be fixed.

> 
> - Sect 4.3 states:
>     pktcSigCompliances - this table has one object that has compliance
>     statements for devices that implement Signaling on the MTA.
> 
>     pktcSigGroups - this table contains group of objects for the 
> common
>     portion of the PacketCable NCS and Signaling MIB.
> 
>     pktcInternationalGroup - this table extends this MIB Module by
>     establishing a set of objects designed to support operations over
>     the widest possible range of markets.
> 
>   while none of these 3 are "tables" in MIB speak.

Ok, s/table/object group - will be fixed.

> - IN de MODULE-IDENTITY DESCRIPTION clause it states:
> 
>            Copyright (C) The Internet Society (2007). This version of
>            this MIB module is part of RFC yyyy; see the RFC itself for
>            full legal notices."
> 
>    That should be the new IETF Trust Copyright statement:
> 
>            Copyright (C) The IETF Trust (2007).  This version of this
>            MIB module is part of RFC yyyy; see the RFC itself for full
>            legal notices."

Ok, will be fixed.

> - pktcNcsEndPntConfigTable and the objects in that table have a prefix

> of
>   pktcNcs.... Why not pktcSigNcs....  ???
>   Just to better avoid any future name clashes in other MIB modules .

I would be fine with this.

> 
> - I see:
>    pktcSigDevEchoCancellation  OBJECT-TYPE
>        SYNTAX       TruthValue
>        MAX-ACCESS   read-only
>        STATUS       current
>        DESCRIPTION
>            " This object specifies if the device is capable of echo
>              cancellation."
>        ::= { pktcSigDevObjects 2 }
> 
>   And so I assume that the value true(1) means that the device is
>   capable of echo cancellation. I think it would be clearer if that
>   were explicitly stated.
> 
>   There are more of such objects of SYNTAX TruthValue that could
>   be clarified in a similar way.
Will do.

> 
> - there are some strange control characters in the document.
>   Specifically in the section titles, for example:
> 
> 
>     9. ^M  IANA Considerations
> 
> - Security Considerations:
> 
>    Even if the network itself is secure (for example by using IPSec),
> 
>   pls change IPSec into IPsec which is the proper spelling and part of

> the
>   latest security template.

Will be fixed.

> 
> ---------------------------
> 
> references issues (found by my own tool):
> 
> !! Missing citation for Informative reference:
>   P072 L048:    [ITU-T-E.180] ITU-T E.180: "Various Tones Used in
> National Networks,
> 
> ---------------------------
> 
> IDnits tells me:
> 
>      (See RFC 3967 for information about using normative references to
>      lower-maturity documents in RFCs)
>   -- Possible downref: Non-RFC (?) normative reference: ref.
> 'ITU-T-J169'
> 
>   -- Possible downref: Non-RFC (?) normative reference: ref.
> 'PKT-SP-CODEC'
> 
>   -- Possible downref: Non-RFC (?) normative reference: ref.
> 'PKT-SP-MGCP'
> 
>   -- Possible downref: Non-RFC (?) normative reference: ref.
> 'PKT-SP-PROV'
> 
> probably OK, just listing it for completeness.

Yes, this is ok.

- S

_______________________________________________
IPCDN mailing list
IPCDN@ietf.org
https://www1.ietf.org/mailman/listinfo/ipcdn