Attention is currently required from: Hoernchen, daniel, laforge.
dexter has posted comments on this change by Hoernchen. ( https://gerrit.osmocom.org/c/pysim/+/43545?usp=email )
Change subject: smpp2sim: make the SCP81 BIP relay work ......................................................................
Patch Set 2: Code-Review+1
(11 comments)
Patchset:
PS2: I have now carefully read that through. I think this patch would benefit a lot from a more verbose documentation. All methods that are important for an API user should have all input and output parameters documented and contain a hint on how they should be used. I know I have left a lot of comments, some reflect what I have understood. Maybe you can use that for a reference to improve the documentation.
Otherwise I think the logic is all right. This should work.
File pySim/bip.py:
https://gerrit.osmocom.org/c/pysim/+/43545/comment/fbb002a2_3adc6228?usp=ema... : PS2, Line 30: # Without them the handler builds and logs what it would send. I would move such documentation into the API docstrings of the class. General information is ok at this location, the specific bits like which parameters the constructors get, etc, that should be in the API docstrings
https://gerrit.osmocom.org/c/pysim/+/43545/comment/d2648cd9_6f51fb19?usp=ema... : PS2, Line 64: bandwidth.. This explains why we have moved away from twisted. I would put this into a normal comment. Having this in the API docstring is a bit too much I think...
https://gerrit.osmocom.org/c/pysim/+/43545/comment/b2d9b943_f029e2e2?usp=ema... : PS2, Line 119: if was_empty and not self._closing: I think its important to document why we only notify when the buffer was empty. As far as I understand the rationale is that when a new event happens the buffer is usually empty.
So data comes in, the notify happens and a ENVELOPE EVENT DOWNLOAD is sent to the card (actively by the API user, via the ENVELOPE command). The card now knows that there was an event and how much data is in the buffer.
Then the card sends proactive RECEIVE DATA commands to get the data from the buffer. When the card has received all data, the buffer is empty again and ready for a new event.
Since with each RECEIVE DATA, the card gets the current amount of remaining bytes, we can be sure that the cards reads all bytes until the buffer really runs empty. So basically we will see burts of incoming data with pauses in between. The ENVELOPE EVENT DOWNLOAD is to get the data transfer to the card going again after there was a pause.
https://gerrit.osmocom.org/c/pysim/+/43545/comment/377bf6d1_1d6fa423?usp=ema... : PS2, Line 126: def rx_available(self) -> int: Missing API doc
(you have take_rx and wait_rx, maybe rename this method to "available_rx" or vice-versa?)
https://gerrit.osmocom.org/c/pysim/+/43545/comment/d4a1deef_429e9510?usp=ema... : PS2, Line 175: def __init__(self, on_data_available=None): I think here it is in particular important to put an API docstring and to document the on_data_available callback in the API docstring.
https://gerrit.osmocom.org/c/pysim/+/43545/comment/9a4cb166_3a92c552?usp=ema... : PS2, Line 189: def channel_delete(self, chan_nr: int): maybe add a docstring?
https://gerrit.osmocom.org/c/pysim/+/43545/comment/687f6ee6_bf03f992?usp=ema... : PS2, Line 192: def notify_data_available(self, chan: ProactChannel): The ProactChannel object is created by the channel_create method in this class. Maybe it makes sense to give the self._on_data_available callback to the constructor of ProactChannel. Then the ProactChannel can call it directly, without reaching back to here.
This would also result in a clearer architecture as it seems the only reason why the ProactChannel object has a reference to the ProactChannels object is to reach the notify_data_available callback. Then we can also just pass the callback directly with the constructor. We then also can drop the notify_data_available method here entirely.
https://gerrit.osmocom.org/c/pysim/+/43545/comment/7ad435c6_e6e4f723?usp=ema... : PS2, Line 199: def __init__(self, data_available_sink=None, sms_sink=None): Explain the parameters in an API docstring. In particular the data_available_sink is important here.
The API user should know what to do with the result. I think the comment is already good, but I would mention that the API user gets the encoded bytes. So all he has to do is to forward those bytes using the ENVELOPE command.
https://gerrit.osmocom.org/c/pysim/+/43545/comment/91cc2b9d_11c38d19?usp=ema... : PS2, Line 268: def build_data_available_envelope(self, chan: ProactChannel) -> bytes: As far as I can see this is called from _on_channel_data_available only. This means that it is an internal method - not to be called by API users. I would recommend to prefix it with an underscore when this is the case.
https://gerrit.osmocom.org/c/pysim/+/43545/comment/80ce1b95_14c01fd5?usp=ema... : PS2, Line 392: def handle_ReceiveData(self, pcmd: ProactiveCommand): maybe add some more API doc on how to use this function? As far as I understood, this is to be called by the API user when he has received a proactive RECEIVE DATA command from the card. So with this the card basically actively fetches data from the rx buffer.