Re: [PATCH 1/6] Revert "mbpi: support for auth NONE"

[Denis Kenzior]

Hi Sergei,

On 9/4/22 03:16, Sergei Golubtsov wrote:

This reverts commit a5bdf48ca7be70a9b33a47dae0ea03bf842efdd2.
---
  plugins/mbpi.c | 6 --
  1 file changed, 6 deletions(-)



All applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Fwd: [PATCH 1/1] quectel: fixing EC200T modem initialization

[Denis Kenzior]

Hi Sergei,

On 9/2/22 02:14, Sergei Golubtsov wrote:

EC200T doesn't indicate that the Phonebook initialization
is completed (==4) if AT+CFUN=4
---
  plugins/quectel.c | 7 ++-
  1 file changed, 6 insertions(+), 1 deletion(-)



I don't think this is going to work:

denkenz@localhost ~/ofono-master $ git am ~/merge/Fwd\:\ \[PATCH\ 1_1\]\ 
quectel\:\ fixing\ EC200T\ modem\ initialization\ -\ Sergei\ Golubtsov\ 
\\ -\ 2022-09-02\ 0214.eml

warning: quoted CRLF detected
Applying: Fwd: [PATCH 1/1] quectel: fixing EC200T modem initialization
error: corrupt patch at line 10
Patch failed at 0001 Fwd: [PATCH 1/1] quectel: fixing EC200T modem 
initialization
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Just use git-send email and resend.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Fwd: [PATCH 0/4] Reverting to use CHAP by default

[Denis Kenzior]

Hi Sergei,

On 9/1/22 09:50, Sergei Golubtsov wrote:

Hi Denis,

Didn't get any response to two series of patches I've sent. Did you see them? I 
sent them both at the same time.




Can you resend these?  For some reason I don't have the actual patch from the 
first series (just the cover letter)


and the second series I'm missing email 2/4 and 4/4.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Understanding set_active_card_slot of the SIM atom

[Denis Kenzior]

Hi,

On 4/12/22 08:43, Uyeop Uyeop wrote:

Hello everyone,

I'm writing a softdriver for a non AT modem and we have a Dual Sim Single Active 
use case. At the moment there is only one example from the atmodem softdriver 
where this interface is being offered.


I'm having some difficulty understanding how it works. From the looks of it, 
/sim_set_slot_callback() /is only setting the /active_card_slot/ information. 
Shouldn't the sim properties be refreshed after switching? The newly activated 
sim card properties (imei, imsi, etc) should be updated to the atom, but I just 
don't see how this happens in the code.


This feature is assumed to work by having the modem perform sim removal and sim 
insertion.  The old SIM in the current active slot is removed and the new SIM in 
the new active slot is inserted.


The above works similarly to any other SIM removal / SIM insertion procedure: 
post_sim/post_online atoms are removed, sim atom wipes out all state related to 
the currently inserted SIM and re-initializes the state once the new SIM is 
inserted.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,



Yes, I propose to revert that commit.



That sounds fine to me.


Do you think that we should keep the following unchanged in this case?

 > As well as ofono/plugins/fileprovision.c::
 >
 >      /* select default authentication method */
 >      (*settings)[0].auth_method = OFONO_GPRS_AUTH_METHOD_NONE;
 >



Not sure?  I think only a few folks use the file-provision plugin.  You can 
always send an RFC patch and see if anyone chimes in.


And, yes. I think that even just only revert of 
a5bdf48ca7be70a9b33a47dae0ea03bf842efdd2 would be enough for my case.




Can you test that this indeed fixes your issue and send a 'git revert' commit 
with a proper commit description?  Essentially just a quick summary of this thread?


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,


The question is really whether it is the network or the modem at fault here.
Does AUTH_METHOD_NONE work on other networks with this hardware?


The HW shows the same result in the network of a different provider which also 
has empty user/pass APN settings. I see that there are many similar providers 
worldwide. Even in the US. T-Mobile for instance 
https://gitlab.gnome.org/GNOME/mobile-broadband-provider-info/-/blob/main/serviceproviders.xml#L13394 
 
.


So from what I recall, we use CHAP by default if a username/password is provided 
and NONE otherwise.  This was added in commit:

a5bdf48ca7be70a9b33a47dae0ea03bf842efdd2

Also the LTE and GPRS contexts have slightly different behaviors.  The 
provisioning plugins (mbpi, file-provision) is only performed for GPRS.  LTE is 
provisioned separately.


Are you proposing that we revert the above commit?  Would that be enough or do 
we need other changes?


Since the DTD for mbpi 
(https://gitlab.gnome.org/GNOME/mobile-broadband-provider-info/-/blob/main/serviceproviders.2.dtd) 
doesn't even list none as a valid authentication type, I would tend to think 
that the commit in question wasn't correct.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] rilmodem: fix a problem that manager_dial_callback is never called

[Denis Kenzior]

Hi Xiaoyi,

On 12/16/21 03:30, Xiaoyi Chen wrote:

This problem does not happen each time when dialing. It's only observed
with some sim cards under certain network.

The time sequence to reproduce the problem is:
- send dial request
- receive unsol event call state changed
- send clcc poll request
- clcc poll response (vd->cb is null here)
- dial response
- setup vd->cb (then it never gets called)
---
  drivers/rilmodem/voicecall.c | 7 +++
  drivers/rilmodem/voicecall.h | 1 +
  2 files changed, 8 insertions(+)

diff --git a/drivers/rilmodem/voicecall.c b/drivers/rilmodem/voicecall.c
index 6c169166..00a1bd9e 100644
--- a/drivers/rilmodem/voicecall.c
+++ b/drivers/rilmodem/voicecall.c
@@ -396,6 +396,7 @@ static void rild_cb(struct ril_msg *message, gpointer 
user_data)
 * DIAL_MODIFIED_TO_DIAL means redirection. The call we will see when
 * polling will have a different called number.
 */
+   vd->suppress_clcc_poll = FALSE;


I added an extra newline here to comply with doc/coding-style.txt item M1


if (message->error == RIL_E_SUCCESS ||
(g_ril_vendor(vd->ril) == OFONO_RIL_VENDOR_AOSP &&
message->error == RIL_E_DIAL_MODIFIED_TO_DIAL)) {
@@ -447,6 +448,7 @@ static void dial(struct ofono_voicecall *vc,
clir);
  
  	/* Send request to RIL */

+   vd->suppress_clcc_poll = TRUE;


I moved this statement inside the if ...


if (g_ril_send(vd->ril, RIL_REQUEST_DIAL, ,
rild_cb, cbd, g_free) > 0)


here to be on the safe side.  There's almost never a time where g_ril_send will 
fail, but setting suppress_clcc_poll should only happen if the command could be 
scheduled successfully.



return;
@@ -593,6 +595,10 @@ void ril_call_state_notify(struct ril_msg *message, 
gpointer user_data)
struct ril_voicecall_data *vd = ofono_voicecall_get_data(vc);
  
  	g_ril_print_unsol_no_args(vd->ril, message);

+   if (vd->suppress_clcc_poll) {
+   ofono_info("suppress clcc poll!");


I changed this ofono_info to a DBG()


+   return;
+   }
  
  	/* Just need to request the call list again */

ril_poll_clcc(vc);
@@ -829,6 +835,7 @@ int ril_voicecall_probe(struct ofono_voicecall *vc, 
unsigned int vendor,
vd->vendor = vendor;
vd->cb = NULL;
vd->data = NULL;
+   vd->suppress_clcc_poll = FALSE;
  
  	clear_dtmf_queue(vd);
  
diff --git a/drivers/rilmodem/voicecall.h b/drivers/rilmodem/voicecall.h

index 31e120e3..beb25104 100644
--- a/drivers/rilmodem/voicecall.h
+++ b/drivers/rilmodem/voicecall.h
@@ -31,6 +31,7 @@ struct ril_voicecall_data {
void *data;
gchar *tone_queue;
gboolean tone_pending;
+   gboolean suppress_clcc_poll;
  };
  
  int ril_voicecall_probe(struct ofono_voicecall *vc, unsigned int vendor,




Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] rilmodem: fix handling both of SUCCESS AND FAILURE

[Denis Kenzior]

Hi JongSeok,

On 12/16/21 04:48, JongSeok Won wrote:

Hi denis,

Here is patch for mulfunctioning when failure of RIL Command,
RIL_REQUEST_SET_UNSOL_CELL_INFO_LIST_RATE, is occurred.



I changed the commit description slightly and..


Best Regards,
JongSeok

---
  drivers/rilmodem/netmon.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/rilmodem/netmon.c b/drivers/rilmodem/netmon.c
index 9f3a1191..f7f81312 100644
--- a/drivers/rilmodem/netmon.c
+++ b/drivers/rilmodem/netmon.c
@@ -358,8 +358,10 @@ static void periodic_update_cb(struct ril_msg *message, 
gpointer user_data)
struct cb_data *cbd = user_data;
ofono_netmon_cb_t cb = cbd->cb;
  
-	if (message->error != RIL_E_SUCCESS)

+   if (message->error != RIL_E_SUCCESS){


Added a space after ')' and before '{' in line with our coding-style.txt 
document.


CALLBACK_WITH_FAILURE(cb, cbd->data);
+   return;
+   }
  
  	CALLBACK_WITH_SUCCESS(cb, cbd->data);

  }



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: drivers/qmimodem/gprs.c:create_wds_cb() ofono_gprs_set_cid_range(gprs, 1, 1) <-- hard coded range doesn't work with all SIM cards

[Denis Kenzior]

Hi Doug,

On 12/15/21 10:55, dburr...@miovision.com wrote:

This hardcoded range of possible CIDs doesn't quite make to me, can someone 
explain this? I've got a Verizon SIM card and its default profile index is 3, 
this falls outside the range set for possible CIDs and therefore can't activate 
the CID. Just wondering if someone can help me understand this and help figure 
out the best way forward so that Verizon SIMs can be activated.


In general CIDs are somewhat meaningless.  At least as far as oFono core is 
concerned.  In 27.007 (AT Commands) it is simply an identifier used to 
synchronize multiple AT commands (+CGDCONT, etc) to configure context settings 
like APN, username, password.  oFono core is not aware of any 'default' cids, it 
always sends the entire set of settings on to the driver when the context is 
activated.


QMI should be similar.  If you have LTE and your provider expects multiple 
contexts to be used (one for IMS/VoLTE/default attach and one for data for 
example), I'm not sure if the QMI driver even supports that.  Patches are always 
welcome of course.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] rilmodem: fix register of unsolicited notify

[Denis Kenzior]

Hi Xiaovi,

On 12/13/21 2:35 AM, Xiaoyi Chen wrote:

---
  drivers/rilmodem/stk.c | 6 +++---
  1 file changed, 3 insertions(+), 3 deletions(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] quectel: get devinfo

[Denis Kenzior]

Hi Sean,


What do you think about this: [0]

In attr_cb() the +10 to attr pointer I need to do better.
While looking at the code, in attr_cb() do we leak the attr string? I
can't find where it's free'd :)


No, the string is declared as const char in both attr_cb and 
at_util_parse_attr().  So it is returning a pointer to a buffer owned by GAtChat.





  static void attr_cb(gboolean ok, GAtResult *result, gpointer user_data)
  {
-   struct cb_data *cbd = user_data;
+   struct custom_cb_data *ccd = user_data;
+   struct cb_data *cbd = ccd->cbd;
ofono_devinfo_query_cb_t cb = cbd->cb;
const char *prefix = cbd->user;
struct ofono_error error;
@@ -57,21 +69,33 @@ static void attr_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
return;
}
  
+	if (ccd->vendor == OFONO_VENDOR_QUECTEL_SERIAL &&

+   (!strcmp(prefix, "+CGMI:") ||
+!strcmp(prefix, "+CGMR:"))) {
+   if (!strncmp("Revision: ", attr, 10)) {
+   cb(, attr+10, cbd->data);
+   return;


https://people.gnome.org/~ryanl/glib-docs/glib-String-Utility-Functions.html#g-str-has-prefix


+   }
+   }
+
cb(, attr, cbd->data);
  }
  





@@ -111,15 +141,18 @@ static void at_query_revision(struct ofono_devinfo *info,
  static void at_query_serial(struct ofono_devinfo *info,
ofono_devinfo_query_cb_t cb, void *data)
  {
-   struct cb_data *cbd = cb_data_new(cb, data);
-   GAtChat *chat = ofono_devinfo_get_data(info);
+   struct custom_cb_data *ccd = g_new0(struct custom_cb_data, 1);
+   struct dev_data *dev = ofono_devinfo_get_data(info);
+   ccd->cbd = cb_data_new(cb, data);
+   ccd->vendor = dev->vendor;


Why not just put everything you need from cb_data into your custom data object 
instead of trying to allocate cb_data.


  
-	cbd->user = "+CGSN:";

+   ccd->cbd->user = "+CGSN:";
  
-	if (g_at_chat_send(chat, "AT+CGSN", NULL, attr_cb, cbd, g_free) > 0)

+   if (g_at_chat_send(dev->chat, "AT+CGSN", NULL, attr_cb, ccd, g_free) > 
0)
return;
  
-	g_free(cbd);

+   g_free(ccd->cbd);
+   g_free(ccd);
  
  	CALLBACK_WITH_FAILURE(cb, NULL, data);

  }





@@ -146,11 +184,11 @@ static int at_devinfo_probe(struct ofono_devinfo *info, 
unsigned int vendor,
  
  static void at_devinfo_remove(struct ofono_devinfo *info)

  {
-   GAtChat *chat = ofono_devinfo_get_data(info);
+   struct dev_data *dev = ofono_devinfo_get_data(info);
  
-	ofono_devinfo_set_data(info, NULL);

+   g_at_chat_unref(dev->chat);


free dev?
  
-	g_at_chat_unref(chat);

+   ofono_devinfo_set_data(info, NULL);
  }
  
  static const struct ofono_devinfo_driver driver = {




Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] quectel: get devinfo

[Denis Kenzior]

Hi Sean,


Good plan :)
I have been looking at it. [0]


Yep, that looks like a good start.



Should I catch the callback directly in atmodem/devinfo.c instead?


You could try to intercept this in attr_cb itself.  But you'd need access to 
vendor selector somehow.  Perhaps a custom data structure instead of cb_data. 
Alternatively store the prefix in struct dev_data and set cb_data->user to 
struct dev_data.


Something like:

static void attr_cb()
{
...

if (at_util_parse_attr(...)) {
...
}

if (dev->vendor == QUECTEL &&
(!strcmp(dev->prefix, ...) ||
!strcmp(dev->prefix, ...))) {
// Strip Revision:
}

...
}



We could also just look for a "Revision: " in query_revision_cb(), but
it's kinda hacky


That's the other approach.  Introduce callback(s) for both revision and 
manufacturer commands and handle the quirk there.  Not sure what else we can do?


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] quectel: get devinfo

[Denis Kenzior]

Hi Sean,

On 12/6/21 3:50 AM, Sean Nyekjaer wrote:

Quectel devices returns "Revision:" before the manufacture and revision.
Via dbus:
"Manufacturer" s "Revision: MTK 0828"
"Revision" s "Revision: M95FAR02A08"
---
  plugins/quectel.c | 2 ++
  1 file changed, 2 insertions(+)



Applied.  But you may want to add a quectel specific quirk handler in the 
devinfo driver that strips the 'Revision: ' prefix.  It should not be part of 
the response.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Upgrade to version 1.32 broken 4G LTE connection

[Denis Kenzior]

Hi Jupiter,

On 11/25/21 12:15 AM, Jupiter wrote:

Hi,

Could anyone advise why the dbus worked fine in 1.30 but it is no
longer working in 1.32? Are there any changes in API?


1.30 is about 2 years old now.  But I don't recall any API changes.  Mostly bug 
fixes and driver improvements.




# dbus-send --system --print-reply --dest=org.ofono /
org.ofono.Manager.GetModems
method return time=1628092736.610971 sender=:1.269 ->
destination=:1.278 serial=7 reply_serial=2
array [
]



Hard to say without seeing ofono logs.  Looks like no modems are detected, which 
could be due to kernel changes, etc.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Race condition with LTE on QMI

[Denis Kenzior]

Hi Andrew,

On 11/15/21 6:37 PM, Andrew Murray wrote:

Hello,

I'm using a SIM7600 USB modem (QMI) with Connman and Ofono, however
I've found that I don't get a connection on first boot. I've debugged
this and have some clues as to why...

On first boot (or every boot with a read-only filesystem), I observe
the following:


So why would you use a read-only filesystem?



ofonod[406]: LTE attach IP type: 0
ofonod[406]: src/gprs.c:ofono_gprs_cid_activated() cid 1
ofonod[406]: src/gprs.c:ofono_gprs_cid_activated() cid 1 activated
before atom registered


Part of the problem is that the modem is going online too quickly.  It would 
probably help if the modem was kept in post-sim state long enough to read SPN, 
provision contexts, etc.


Not sure if all the drivers were updated to put gprs atom creation into 
post_sim.  Some might still put it into post_online.  So that is another thing 
to check.




This occurs because of a bail condition
(!__ofono_atom_get_registered(gprs->atom)) - the git commit for this
("gprs: Ignore activated contexts during init, list them later")
suggests that this was added to avoid duplicate contexts in the case
where a context gets activated before provisioning (perhaps in the
case of LTE). The commit also calls driver->list_active_contexts to
handle this - however list_active_contexts has only been implemented
for the atmodem driver.

Therefore should the qmi driver be updated to implement a
list_active_contexts call that will run get_default_profile_cb and
then share this via ofono_gprs_cid_activated ?


That should certainly help since list_active_contexts was added for this 
purpose.



Without this we seem to get a race condition. In the function
drivers/qmimodem/gprs.c:create_wds_cb we rely on the callback related
to QMI_NAS_GET_SS_INFO to eventually call ofono_gprs_cid_activated -
however this must happen before ofono_gprs_register (also in
reate_wds_cb) completes.

My observation is that on first boot, there are no settings in
/var/lib/ofono, thus ofono_sim_add_spn_watch is called to provision
contexts - this takes time and results in the "activated before atom
registered" error. If I kill ofono and start it again - then this time
there are settings, gprs_load_settings no longer returns NULL and so
ofono_gprs_register returns quicker - resulting in everything working.


Context settings are stored on disk, so provisioning is no longer needed.  Also 
most SIM reads are no longer performed since they are now cached in the 
filesystem.  This should generally result in almost no delay in gprs atom 
registration.  Hence my question above about the use of a read-only filesystem.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

Hi Alex,

Yeah I think you are right and I am getting really irritated with them giving me 
the run-around tbh.




I sympathize, been there ;)



Yeah ok - what name do you suggest?


So there's a few things you can do actually:
- We do have g_at_result_next_unquoted_string.  You could try using that to 
obtain the negative  and then just sscanf/strtol or whatever to convert it.
- We also have g_at_result_iter_raw_line() which can give you the raw result 
line.  It should be trivial to come up with some sscanf magic to parse the result.
- Just add a new g_at_result_iter_next_negative_number.  Or if you come up with 
a more descriptive word than negative, feel free.




Also there's a bigger problem here I think, which is that if I parse negative 
values and try to pass them back via e.g. netmon interface then Ofono is 
treating anything negative in there as an error.


oFono uses 27.007 as the interface between the driver and the core (and also for 
DBus API).  If it isn't defined in 27.007, then your driver has to sanitize it 
to a range that is defined in 27.007.  In your  case any negative value 
would just be converted to a '0' which means < -140.




And the DBUS implementation is unsigned for values based on the thinking that AT 
responses are non-negative.




See above, the DBus implementation would be untouched.  If you need to 
*actually* know whether  is -142 or other range that is not currently 
supported by 3GPP 27.007, then that is a bit nastier problem.


So actually there are a number of invasive changes that seem to need to be made 
to support negative numbers?




Hopefully not?

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

Hi Alex,



OK - they say it's fine and you say it's not. I don't have the time to be the 
middle-man here.




I'm fairly certain your vendor's firmware is not following the relevant 
standards, but it sounds like they don't want to fix it.  What can I do? :)


Although I'd like to, clearly I won't be able to contribute this Quectel support 
upstream.




Why does it even matter?  I told you very early on that it would be acceptable 
to add a new g_at_result_* method that handles negative integers.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

Hi Alex,


Denis - I am being directed to the ITU 36.133 spec here

https://portal.3gpp.org/desktopmodules/Specifications/SpecificationDetails.aspx?specificationId=2420



Sure, same document/section where 27.007 refers to for .  But 
unfortunately 27.007 only defines the range 0..97 where 0 is  < 140 dbm. 
How 3GPP decides to represent  values between 156 and 140 is up to 3GPP. 
If your vendor is using negative integers in AT commands, then that is a vendor 
extension and not something that is permitted by ITU v.250.



This document defines this in 9.1.4

I can't see where RSRP_-17 is defined anywhere but the strong implication to me 
is that this number should be -17


Can you confirm where it states that these numbers cannot be negative numbers as 
I cannot find that requirement.




V.250 Section 5.3.1.  Notice that 27.007 never uses negative integers, there's a 
reason for that.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

Hi Alex,

I have been chasing this up with Quectel and they tell me this the correct 
implementation.


"QCOPS and QENG command show the RSSI、RSRP and SINR value of the network, this 
is in accordance with the 3GPP TS regulations, so it is normal for negative 
numbers to be displayed.image"



So the image shows the value for 

'0' if -140 dbm
...
'95' if dbm is -46 to -45
etc.

 (what is actually sent over AT commands) is never negative. Isn't the 
image supporting my earlier assertion:  'AT commands do not have a concept of 
negative numbers'?


oFono exposes the  value from 3GPP:

"byte ReferenceSignalReceivedPower [optional, lte]

Contains the Reference Signal Received Power.  Valid range of values
is 0-97. Refer to  in 27.007, Section 8.69 for more details.
"

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] simfs: Fix reads beyond the first block

[Denis Kenzior]

Hi Slava,

On 7/30/21 10:52 AM, Slava Monich wrote:

On 30/07/2021 18.37, Denis Kenzior wrote:

Hi Slava,

On 7/30/21 7:07 AM, Slava Monich wrote:

---
  src/simfs.c | 17 -
  1 file changed, 8 insertions(+), 9 deletions(-)



Funny how long this bug has been lurking around.



Until we finally had a crash on reading an icon or something out of SIM. Which 
most SIMs apparently don't have or else it would've been noticed earlier.




Figured :)




diff --git a/src/simfs.c b/src/simfs.c
index 3d4f6283..cf770265 100644
--- a/src/simfs.c
+++ b/src/simfs.c
@@ -383,18 +383,18 @@ static void sim_fs_op_read_block_cb(const struct 
ofono_error *error,

  }
    start_block = op->offset / 256;
-    end_block = (op->offset + (op->num_bytes - 1)) / 256;
+    end_block = op->num_bytes ? (op->offset + op->num_bytes - 1) / 256 :
+    start_block;


Curious why this is needed?  op->num_bytes should never be zero since it gets 
set to the file length?




I admit that it's a bit paranoid, but op->num_bytes is assigned without checking 
and I figured that it wouldn't hurt to do a check here. Feel free to drop this 
part if it looks like too much of an overkill to you.




I'm all for being paranoid, but there's not much sense doing a round-trip to the 
SIM for a 0 byte read.  So I rather we catch this elsewhere.


There's some sanity checking of num_bytes being 0 once the file info has been 
obtained.  It should be reset to file length if it is zero, which is generally 
what we want for simple binary files.


Invocations of ofono_sim_read_bytes() should probably sanity check the length if 
they want to be fully paranoid.


I went ahead and applied this patch without this particular chunk.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] simfs: Fix reads beyond the first block

[Denis Kenzior]

Hi Slava,

On 7/30/21 7:07 AM, Slava Monich wrote:

---
  src/simfs.c | 17 -
  1 file changed, 8 insertions(+), 9 deletions(-)



Funny how long this bug has been lurking around.


diff --git a/src/simfs.c b/src/simfs.c
index 3d4f6283..cf770265 100644
--- a/src/simfs.c
+++ b/src/simfs.c
@@ -383,18 +383,18 @@ static void sim_fs_op_read_block_cb(const struct 
ofono_error *error,
}
  
  	start_block = op->offset / 256;

-   end_block = (op->offset + (op->num_bytes - 1)) / 256;
+   end_block = op->num_bytes ? (op->offset + op->num_bytes - 1) / 256 :
+   start_block;
  


Curious why this is needed?  op->num_bytes should never be zero since it gets 
set to the file length?


Rest looks good to me.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Documentation on when to free structures in/after callbacks

[Denis Kenzior]

Hi Alex,

On 7/30/21 9:24 AM, ajlen...@dynamicdevices.co.uk wrote:

Thanks Denis that helps. I see I need to learn a bit and get my head around 
this.

For example I find this really confusing:

   cbd = req_cb_data_ref(cbd);
 if (g_at_chat_send(nmd->chat, "AT+CESQ", cesq_prefix,
 cesq_cb, cbd, req_cb_data_unref) == 0) {
 CALLBACK_WITH_FAILURE(cbd->cb, cbd->data);
 req_cb_data_unref(cbd);
 }

This does some kind of allocation - ok
This makes the call - ok fine
Provides the unref function into the call - ok so far



So in this particular case, g_at_chat_send() returning 0 means the command could 
not be scheduled for whatever reason (usually the tty port is dead).  By 
convention, failures do not side-effect.  So you have to free any allocated 
resources manually.  Hence the unref here.



But then I'm thinking if the call _succeeds_ our unref gets called?


Yes.  destroy callback is called in every circumstance once g_at_chat_send() 
returns a success.  So it will get called after cesq_cb(), or if the request is 
cleaned up early (cesq_cb() never called)



But if our call _fails_ then we have to do the unref ourselves?


Right, the 'no-side-effect' part applies.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Documentation on when to free structures in/after callbacks

[Denis Kenzior]

Hi Alex,

On 7/30/21 8:21 AM, ajlen...@dynamicdevices.co.uk wrote:

I'm not sure what you mean? What exactly do you think needs freeing?


Strange. I am lookinga at the drivers and sometimes unref functions are 
provided into other functions and sometimes not


So you're talking about callbacks in the driver itself, not actually crossing 
the core<->driver boundary.


There aren't really any docs, but g_at_chat acts just like any other GLib based 
library (or many main-loop driven C libraries for that matter), so pretty much 
the same rules apply.  I can give you some hints...




 if (g_at_chat_send(nmd->chat, "AT+QENG=\"servingcell\"", qeng_prefix,
 qeng_cb, cbd, req_cb_data_unref) == 0) {




Under the hood, g_at_chat_send puts a request on the queue.  And the request 
might not be processed until some time later.  It can also be interrupted and 
cleaned up without the callback being called (if the hardware gets pulled for 
example).  So if you're passing any allocated userdata, you need to provide a 
'destroy' method to de-allocate it, otherwise it will leak.


In your example, cbd is userdata and req_cb_data_unref is the destroy method, 
respectively.


Oh, and req_cb_data is a common pattern.  It is used where you need to run 
several commands in sequence (think of it as a transaction) and the allocated 
userdata needs to be cleaned up even if the transaction is interrupted.



Then these CALLBACK_WITH_FAILURE and CALLBACK_WITH_SUCCESS calls sometimes seem 
to have free functions after them and sometimes not.



There is definitely a reason why they do.  Hopefully the explanation above 
helps.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Documentation on when to free structures in/after callbacks

[Denis Kenzior]

Hi Alex,

On 7/28/21 11:32 AM, ajlen...@dynamicdevices.co.uk wrote:

Hi all,

I'm trying to write a driver for Quectel modems as I need some specific bits 
and bobs - mainly signal strengths for the serving cell and neighbourhood cells

I can't quite get my head around when I am supposed to provide free() functions 
to to calls and when to free in callbacks on success or error conditions.


I'm not sure what you mean?  What exactly do you think needs freeing?

Everything from the driver going to the core are const * or passed by value. 
There should be no instances of the core taking ownership of any memory from the 
driver.  To put another way, if the driver mallocs something, then the driver is 
responsible for freeing it.


Similarly, everything being passed in from the core to the driver is a const * 
or passed by value.


Fundamentally, there should never be any resource ownership change between 
driver and core...




Is there a bit of a document somewhere somebody could direct me to that 
explains this?

Or maybe I am overthinking and somebody could just explain the logic behind it?

Thanks!

Alex
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Bug/Oversight in gatchat/gatresult.c with negative numbers

[Denis Kenzior]

Hi Alex,

On 7/20/21 11:11 AM, Alex J Lennon wrote:

Hi all,

I'm doing a bit of work with Ofono again, extending support for a Quectel EG91 - 
handling additional cell strength information which seems necessary for LTE.


I'm using Quectel's AT+QCOPS which is returning some negative signal strengths 
and after some investigation I've spotted that 
gatresult::g_at_result_iter_next_number() doesn't handle negative numbers.


AT commands do not have a concept of negative numbers.  v.250 Section 5.3.1:
 may be a string of one or more characters from "0" through "9" 
representing a decimal
integer value. Commands that expect a  are noted in the description of 
the command (see

clause 6).

I don't recall any other vendor using negative values in AT commands.

If you want to add a negative number parser to GAtChat, then please make it a 
separate method, so that it is very clear that a vendor 'extension' is being 
worked around.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] gemalto: radio-settings: cleanup

[Denis Kenzior]

Hi Sergey,

On 7/23/21 12:00 PM, Sergey Matyukevich wrote:

Enum ofono_radio_access_mode has been replaced by unsigned int.
This change allows to move handling of all the modes into
'switch' in the function gemalto_set_rat_mode.
---
  drivers/gemaltomodem/radio-settings.c | 15 ++-
  1 file changed, 6 insertions(+), 9 deletions(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 2/3] plugins: gemalto: add radio-settings atom

[Denis Kenzior]

Hi Sergey,

On 7/4/21 10:53 AM, Sergey Matyukevich wrote:

Instantiate Gemalto radio-settings atom in post_sim.

---
  plugins/gemalto.c | 2 ++
  1 file changed, 2 insertions(+)



Patch 2 & 3 applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/3] gemalto: add radio-settings driver

[Denis Kenzior]

Hi Sergey,

On 7/4/21 10:53 AM, Sergey Matyukevich wrote:

Add support for Gemalto specific radio settings.

---
  Makefile.am   |   1 +
  drivers/gemaltomodem/gemaltomodem.c   |   2 +
  drivers/gemaltomodem/gemaltomodem.h   |   3 +
  drivers/gemaltomodem/radio-settings.c | 267 ++
  4 files changed, 273 insertions(+)
  create mode 100644 drivers/gemaltomodem/radio-settings.c



I went ahead and applied this with a 2 lines amended: changing 'enum 
ofono_access_mode' to 'unsigned int'.  I recently updated all the drivers in 
commit 1d86dbc6 ("radio-settings: Do not use enum ofono_access_mode").  So I did 
a similar amend here for consistency.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

[ANNOUNCE] IRC moved to OFTC

[Denis Kenzior]

Hello,

Due to recent changes on freenode IRC network, we have decided to move the IRC 
channel to OFTC.  Please update your IRC clients to the new channel at 
irc://irc.oftc.net/#ofono.


We will keep the #ofono channel on Freenode for now to help with the transition.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 2/3] radio-settings: Add handling of dual mode technology preference

[Denis Kenzior]

Hi Sean,

On 5/31/21 6:49 AM, Sean Nyekjaer wrote:

Allow setting of "lte,gsm" mode,
for modems that doesn't support ANY mode.
---
  src/radio-settings.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/src/radio-settings.c b/src/radio-settings.c
index 7283aa98..e2e5f33b 100644
--- a/src/radio-settings.c
+++ b/src/radio-settings.c
@@ -79,6 +79,9 @@ static const char *radio_access_mode_to_string(enum 
ofono_radio_access_mode m)
if (m == (OFONO_RADIO_ACCESS_MODE_LTE|OFONO_RADIO_ACCESS_MODE_UMTS))
return "lte,umts";
  
+	if (m = (OFONO_RADIO_ACCESS_MODE_LTE|OFONO_RADIO_ACCESS_MODE_GSM))


I think you meant '==' here, so I fixed that.


+   return "lte,gsm";
+
return NULL;
  }
  
@@ -104,6 +107,9 @@ static gboolean radio_access_mode_from_string(const char *str,

} else if (g_str_equal(str, "lte,umts")) {
*mode = 
OFONO_RADIO_ACCESS_MODE_LTE|OFONO_RADIO_ACCESS_MODE_UMTS;
return TRUE;
+   } else if (g_str_equal(str, "lte,gsm")) {
+   *mode = OFONO_RADIO_ACCESS_MODE_LTE|OFONO_RADIO_ACCESS_MODE_GSM;
+   return TRUE;
}
  
  	return FALSE;




All three patches applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,

On 5/19/21 6:35 PM, Sergei Golubtsov wrote:

Hi Denis,

Thank you very much for your quick and helpful reply.

The problem is a bit vulgar actually. That's a shame that I took your
time for that but I have another question as I am not sure about the
findings below.


no worries.



I had tried to get dump with gsmdial and gsmdial successfully
activated the context. So the problem is that we have authentication
method set to NONE in ofono. But gsmdial uses CHAP by default.
I have manually set the auth to CHAP for the context in ofono and
ofono successfully activated the context.


Yes, oFono used to use CHAP by default, but this was changed a few years ago.


There is the provider info from mobile-broadband-provider-info about
my provider:

 
 China Unicom
 
 
 
 
 
 uninet
 
 
 
 联通彩信
 http://mmsc.myuni.com.cn
 10.0.0.172:80
 
 
 


Am I correct that ofono must use CHAP if the auth method is not
specified in the db?


We used to do this, then around Sep-Oct 2018 there was a strong push to add an 
explicit 'No Authentication' option for oFono from one of the modem hardware 
vendors.  I don't recall the exact arguments, search the archives, but the 
fallout was that an empty username / password implied no authentication.  Which 
I think does make a certain amount of sense.


git show 6cf24fe1f9cfa2a61422ad84abfdd32e7ea2cf78.  Look around at the commits 
from the same author.  I do seem to recall that 3GPP mandated CHAP to be used as 
the default, even in the no username / password case.  But my memory is fuzzy 
now, and there hasn't been any problems reported since then (until now).




I see that the technology used by the modem is HSPA. And I see the
following in ofono/drivers/atmodem/lte.c:

 /* change the authentication method if the  parameters are invalid */
 if (!*ldd->pending_info.username || !*ldd->pending_info.password)
auth_method = OFONO_GPRS_AUTH_METHOD_NONE;


And in ofono/plugins/mbpi.c:

 /* select authentication method NONE if fit */
 if (!ap->username || !ap->password)
 ap->auth_method = OFONO_GPRS_AUTH_METHOD_NONE;


As well as ofono/plugins/fileprovision.c::

 /* select default authentication method */
 (*settings)[0].auth_method = OFONO_GPRS_AUTH_METHOD_NONE;


ofono/src/lte.c:

 /* this must have a valid default */
if (!gprs_auth_method_from_string(auth_method_str,
>info.auth_method))
lte->info.auth_method = OFONO_GPRS_AUTH_METHOD_NONE;


I am not sure about the standards which may be relevant here. Sorry
for asking this but I thought that we should use CHAP by default,
don't we?


The question is really whether it is the network or the modem at fault here. 
Does AUTH_METHOD_NONE work on other networks with this hardware?




Thank you again and have a nice day.

Yours sincerely,
Sergei Golubtsov.



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,

On 5/18/21 4:19 PM, Sergei Golubtsov wrote:

Hi Denis and all,

I have some problems with the context activation on Quectel EC200T USB in 
chinese network although in russian networks the modem works fine.


I have the following during the activation:

2021-05-18T23:35:50.624972+03:00 ofonod[999]: [info] Modem: < \r\nCONNECT\r\n
2021-05-18T23:35:50.625854+03:00 ofonod[999]: [debug] 
../git/ofono/drivers/atmodem/gprs-context.c:at_cgdata_cb() ok 1
2021-05-18T23:35:50.626564+03:00 ofonod[999]: [debug] 
../git/ofono/drivers/atmodem/gprs-context.c:setup_ppp()
2021-05-18T23:35:50.627463+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_generate_event: current state 0:INITIAL
2021-05-18T23:35:50.628216+03:00 ofonod[999]: [info] PPP: event: 0 (Up), action: 
2, new_state: 2 (CLOSED)
2021-05-18T23:35:50.628884+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_generate_event: current state 2:CLOSED
2021-05-18T23:35:50.629521+03:00 ofonod[999]: [info] PPP: event: 2 (Open), 
action: 1026, new_state: 6 (REQSENT)
2021-05-18T23:35:50.630234+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_initialize_restart_count: current state 2:CLOSED
2021-05-18T23:35:50.630876+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_send_configure_request: current state 2:CLOSED
2021-05-18T23:35:50.631641+03:00 ofonod[999]: [info] PPP: 
../git/ofono/gatchat/gatppp.c:ppp_enter_phase() 1
2021-05-18T23:35:50.634253+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_process_configure_request: current state 6:REQSENT
2021-05-18T23:35:50.635270+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_generate_event: current state 6:REQSENT
2021-05-18T23:35:50.635423+03:00 ofonod[999]: [info] PPP: event: 7 (RCR-), 
action: 4006, new_state: 6 (REQSENT)


Peer is sending us options we do not find acceptable...  Do you have a wireshark 
trace to see what this might be?


2021-05-18T23:35:50.635544+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_send_configure_nak: current state 6:REQSENT
2021-05-18T23:35:50.636566+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_process_configure_ack: current state 6:REQSENT


Looks like our options (that we're sending) are accepted by the peer

2021-05-18T23:35:50.636696+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_generate_event: current state 6:REQSENT
2021-05-18T23:35:50.637067+03:00 ofonod[999]: [info] PPP: event: 8 (RCA), 
action: 27, new_state: 7 (ACKRCVD)
2021-05-18T23:35:50.637187+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_initialize_restart_count: current state 6:REQSENT
2021-05-18T23:35:50.637187+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_initialize_restart_count: current state 6:REQSENT
2021-05-18T23:35:50.637597+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_process_configure_request: current state 7:ACKRCVD
2021-05-18T23:35:50.637715+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_generate_event: current state 7:ACKRCVD
2021-05-18T23:35:50.637826+03:00 ofonod[999]: [info] PPP: event: 7 (RCR-), 
action: 4007, new_state: 7 (ACKRCVD)
2021-05-18T23:35:50.637929+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_send_configure_nak: current state 7:ACKRCVD
2021-05-18T23:35:50.639907+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_process_configure_request: current state 7:ACKRCVD
2021-05-18T23:35:50.640246+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_generate_event: current state 7:ACKRCVD
2021-05-18T23:35:50.640992+03:00 ofonod[999]: [info] PPP: event: 7 (RCR-), 
action: 4007, new_state: 7 (ACKRCVD)


and basically we get into an infinite loop with the peer trying to set some 
options we find unacceptable..




So, the following lines repeats infinitely, context is not activated and ofono 
uses ~90% of the CPU until I stop it:
2021-05-18T23:35:50.637929+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_send_configure_nak: current state 7:ACKRCVD
2021-05-18T23:35:50.639907+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_process_configure_request: current state 7:ACKRCVD
2021-05-18T23:35:50.640246+03:00 ofonod[999]: [info] PPP: lcp: 
pppcp_generate_event: current state 7:ACKRCVD
2021-05-18T23:35:50.640992+03:00 ofonod[999]: [info] PPP: event: 7 (RCR-), 
action: 4007, new_state: 7 (ACKRCVD)


Could someone shed a light on what is going on there and how to solve that?

Thanks in advance.
Have a nice day.

Yours sincerely,
Sergei Golubtsov.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v5 0/4]

[Denis Kenzior]

Hi Jonas,

On 5/12/21 4:17 AM, Jonas Bonn wrote:

Hi Denis,

If we're going to get anywhere with this, the onus probably falls on you to do 
some cleanup work on these patches.  I'll write some comments on the patches and 
maybe you can munge those into the commit messages...?


What now? ;)

Anyway, I am not taking this set since all patches seem to be whitespace damaged 
and this needs cleanup anyway.




Bing Jupiter has been running this modem (uBlox SARA R4) for well over a year 
with these patches so they are somewhat tested.  That said, this modem isn't 
without issues, but it appears to me that those are mostly firmware related at 
this point.  Yeah, not great advertising for the product; probably best to stay 
away from it altogether, really...


This is driver code, so unless there's something obviously wrong I'm fine taking 
it.  But it should at least comply with doc/coding-style.txt and someone should 
have ran checkpatch.pl against it to check for issues.


Given that a v6 is needed, do you want to work with Jupiter to make the 
editorial changes to the commit descriptions?


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] sim-auth: Parse auth response according to TS 31.102

[Denis Kenzior]

Hi Slava,

On 5/3/21 9:04 AM, Slava Monich wrote:

---


So what is this trying to fix?  I mean I can understand 1 line patch fixes that 
come without a description.  These tend to be self-evident.  300+ line diff 
needs a least some explanation.



  src/sim-auth.c  |  34 ++-
  src/simutil.c   | 146 +---
  src/simutil.h   |  13 ++--
  unit/test-simutil.c | 125 -
  4 files changed, 234 insertions(+), 84 deletions(-)


This really should be broken down into at least 2 commits. See HACKING, 
'Submitting Patches' section, item 3.




diff --git a/src/sim-auth.c b/src/sim-auth.c
index 3c3f35e7..6dab52ee 100644
--- a/src/sim-auth.c
+++ b/src/sim-auth.c
@@ -207,14 +207,10 @@ static void handle_umts(struct ofono_sim_auth *sa, const 
uint8_t *resp,
DBusMessage *reply = NULL;
DBusMessageIter iter;
DBusMessageIter dict;
-   const uint8_t *res = NULL;
-   const uint8_t *ck = NULL;
-   const uint8_t *ik = NULL;
-   const uint8_t *auts = NULL;
-   const uint8_t *kc = NULL;
+   struct data_block res, ck, ik, auts, sres, kc;
  
  	if (!sim_parse_umts_authenticate(resp, len, , , ,

-   , ))
+   , , ))
goto umts_end;
  
  	reply = dbus_message_new_method_return(sa->pending->msg);

@@ -224,15 +220,23 @@ static void handle_umts(struct ofono_sim_auth *sa, const 
uint8_t *resp,
dbus_message_iter_open_container(, DBUS_TYPE_ARRAY,
"{say}", );
  
-	if (auts) {

-   append_dict_byte_array(, "AUTS", auts, 14);
-   } else {
-   append_dict_byte_array(, "RES", res, 8);
-   append_dict_byte_array(, "CK", ck, 16);
-   append_dict_byte_array(, "IK", ik, 16);
-   if (kc)
-   append_dict_byte_array(, "Kc", kc, 8);
-   }
+   if (auts.data)
+   append_dict_byte_array(, "AUTS", auts.data, auts.len);
+
+   if (sres.data)
+   append_dict_byte_array(, "SRES", sres.data, sres.len);


You're modifying the behavior of the D-Bus API without actually updating the 
relevant docs.


Also, this part looks suspect.  SRES is only done in GSM context, so shouldn't 
this be taken care sim_parse_gsm_authenticate()?



+
+   if (res.data)
+   append_dict_byte_array(, "RES", res.data, res.len);
+
+   if (ck.data)
+   append_dict_byte_array(, "CK", ck.data, ck.len);
+
+   if (ik.data)
+   append_dict_byte_array(, "IK", ik.data, ik.len);
+
+   if (kc.data)
+   append_dict_byte_array(, "Kc", kc.data, kc.len);
  
  	dbus_message_iter_close_container(, );
  
diff --git a/src/simutil.c b/src/simutil.c

index 5d2aa6a2..30b76f05 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -1672,63 +1672,135 @@ int sim_build_gsm_authenticate(unsigned char *buffer, 
int len,
return build_authenticate(buffer, rand, NULL);
  }
  
-gboolean sim_parse_umts_authenticate(const unsigned char *buffer,

-   int len, const unsigned char **res, const unsigned char **ck,
-   const unsigned char **ik, const unsigned char **auts,
-   const unsigned char **kc)
+gboolean sim_parse_umts_authenticate(const unsigned char *buffer, int len,
+   struct data_block *res, struct data_block *ck,
+   struct data_block *ik, struct data_block *auts,
+   struct data_block *sres, struct data_block *kc)


At least by my reading of 33.102, lengths of ck, ik, kc and sres are set in 
stone, are they not?


Also, RFC 4187 also sets in stone the definition of AUTS (and RES, AUTN, etc). 
So you probably have to explain how and when AUTS length could be not in line 
with that RFC?  The authentication API is mostly meant to be used by 
implementations of this RFC, so we would need to take appropriate steps that 
nothing is broken by this change.



  {
-   if (len < 16 || !buffer)
+   const unsigned char *ptr = buffer;
+   const unsigned char *end = ptr + len;
+   unsigned int l;
+
+   if (!buffer || len < 2)
return FALSE;
  
-	switch (buffer[0]) {

+   memset(res, 0, sizeof(*res));
+   memset(ck, 0, sizeof(*ck));
+   memset(ik, 0, sizeof(*ik));
+   memset(kc, 0, sizeof(*kc));
+   memset(auts, 0, sizeof(*auts));
+   memset(sres, 0, sizeof(*sres));


Generally we don't like side-effecting arguments on error.  If you're going 
through the trouble of introducing a new structure, might as well just have it 
represent the result of the parse operation and cut down on arguments.  Maybe 
something like:


struct sim_auth_umts_result {
uint8_t success;
union {
struct {
const unsigned char *auts;
uint8_t auts_len;
}
struct {
const unsigned char 

Re: [PATCH] simutil: Fill unused part of AID with FFs

[Denis Kenzior]

Hi Slava,

On 4/29/21 11:09 AM, Slava Monich wrote:

Correct handling of short AIDs will take more than that, but
leaving part of the array uninitialized is wrong in any case.
---
  src/simutil.c | 1 +
  1 file changed, 1 insertion(+)

diff --git a/src/simutil.c b/src/simutil.c
index 5d2aa6a2..e648c918 100644
--- a/src/simutil.c
+++ b/src/simutil.c
@@ -1588,6 +1588,7 @@ GSList *sim_parse_app_template_entries(const unsigned 
char *buffer, int len)
goto error;
  
  		memcpy(app.aid, aid, app.aid_len);

+   memset(app.aid + app.aid_len, 0xff, 16 - app.aid_len);


Would it not be easier to fix sim-auth to take aid_len into account instead of 
hard-coding 16?  It seems like sim_auth_register is the only one affected, but I 
didn't look deeply.


  
  		app.type = (app.aid[5] << 8) | app.aid[6];
  



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] build: require glib >= 2.60

[Denis Kenzior]

On 3/8/21 3:33 AM, JongSeok Won wrote:

g_utf8_validate_len() is support after glib 2.60
---
  configure.ac | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/2] sim-auth: Remove watch if open_channel fails

[Denis Kenzior]

Hi Slava,

On 4/26/21 3:17 AM, Slava Monich wrote:

Otherwise open_channel won't be called again after a failure.
---
  src/sim-auth.c | 2 ++
  1 file changed, 2 insertions(+)



Both applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] build: require glib >= 2.60

[Denis Kenzior]

Hi,


-PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.32, dummy=yes,
- AC_MSG_ERROR(GLib >= 2.32 is required))
+PKG_CHECK_MODULES(GLIB, glib-2.0 >= 2.60, dummy=yes,
+ AC_MSG_ERROR(GLib >= 2.60 is required))


This patch doesn't apply:
denkenz@localhost ~/ofono-master $ git am ~/merge/\[PATCH\]\ build\:\ require\ 
glib\ \>\=\ 2.60.eml

Applying: build: require glib >= 2.60
error: corrupt patch at line 12
Patch failed at 0001 build: require glib >= 2.60
hint: Use 'git am --show-current-patch=diff' to see the failed patch
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

Looks like it is whitespace-mangled or otherwise damaged for some reason.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v1 5/5] Use tabs as Denis required

[Denis Kenzior]

Hi Jupiter,

On 2/22/21 10:46 PM, Bing Jupiter wrote:

From: Jupiter 


Please make sure your authorship information is updated in the commit meta-data 
as well for  all the patches you're submitting.




---
  drivers/atmodem/vendor.h |  2 +-
  drivers/qmimodem/gprs.c  | 26 +-
  plugins/gobi.c   | 14 +++---
  plugins/udevng.c |  6 +++---
  4 files changed, 24 insertions(+), 24 deletions(-)



No, please don't do it this way.  This patch should not  exist.  Instead take 
the changes from this patch and amend the previous 4 patches.  In other words, 
the previous 4 patches should be following the coding style as documented in 
doc/coding-style.txt.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/4] SARA R4 QMI support

[Denis Kenzior]

Hi Jupiter,

On 2/22/21 4:14 AM, Jupiter wrote:

From: jupiter 


Please provide a complete and properly capitalized real name with your 
submission, otherwise I can't apply it.




---
  plugins/udevng.c | 41 +
  1 file changed, 41 insertions(+)

diff --git a/plugins/udevng.c b/plugins/udevng.c
index 34ac1cc0..63d95055 100644
--- a/plugins/udevng.c
+++ b/plugins/udevng.c
@@ -1194,6 +1194,45 @@ static gboolean setup_ublox(struct modem_info *modem)
return TRUE;
  }
  
+static gboolean setup_ubloxqmi(struct modem_info *modem) {

+   const char *qmi = NULL, *net = NULL, *gps = NULL, *aux = NULL;
+   GSList *list;
+
+   DBG("%s", modem->syspath);
+
+   for (list = modem->devices; list; list = g_slist_next(list)) {
+   struct device_info *info = list->data;
+
+   DBG("%s %s %s %s %s", info->devnode, info->interface,
+   info->number, info->label, info->subsystem);
+
+   if (g_strcmp0(info->interface, "255/255/255") == 0 &&
+   g_strcmp0(info->number, "03") == 0) {
+   if (g_strcmp0(info->subsystem, "net") == 0)
+   net = info->devnode;
+   else if (g_strcmp0(info->subsystem, "usbmisc") == 0)
+   qmi = info->devnode;
+   }
+   }
+
+   DBG("qmi=%s net=%s", qmi, net);
+
+   if (qmi == NULL || net == NULL)
+   return FALSE;
+
+   DBG("qmi=%s net=%s", qmi, net);
+
+   ofono_modem_set_string(modem->modem, "Device", qmi);
+   ofono_modem_set_string(modem->modem, "NetworkInterface", net);
+ofono_modem_set_string(modem->modem, "Quirk", "SARAR4");


you seem to be mixing tabs and spaces for indentation.  Please fix that


+
+   DBG("gps=%s aux=%s", gps, aux);
+
+   ofono_modem_set_driver(modem->modem, "gobi");
+
+   return TRUE;
+}
+
  static gboolean setup_gemalto(struct modem_info* modem)
  {
const char *app = NULL, *gps = NULL, *mdm = NULL,
@@ -1441,6 +1480,7 @@ static struct {
{ "quectel",  setup_quectel   },
{ "quectelqmi",   setup_quectelqmi},
{ "ublox",setup_ublox },
+{ "ubloxqmi",   setup_ubloxqmi  },


Also here


{ "gemalto",  setup_gemalto   },
{ "xmm7xxx",  setup_xmm7xxx   },
{ "mbim", setup_mbim  },
@@ -1850,6 +1890,7 @@ static struct {
{ "ublox","cdc_ncm","1546", "110a"  },
{ "ublox","rndis_host", "1546", "1146"  },
{ "ublox","cdc_acm","1546", "1146"  },
+{ "ubloxqmi",   "qmi_wwan", "05c6", "90b2"  },


and here


{ "gemalto",  "option", "1e2d",   "0053"},
{ "gemalto",  "cdc_wdm","1e2d",   "0053"},
{ "gemalto",  "qmi_wwan",   "1e2d",   "0053"},



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] qmimodem: Add USSD indication support

[Denis Kenzior]

Hi Alexey,

On 2/19/21 9:30 AM, Alexey Andreyev wrote:

Sorry for a noob html format, retrying from a custom client as plaintext:

Hello! My first patch, feel free to criticize :) This is to handle the USSD 
Indication messages with the qmimodem driver.
Higher-level KDE Plasma Mobile issue: 
https://invent.kde.org/plasma-mobile/plasma-dialer/-/merge_requests/33
Tested with Quectel EG25-G (Pinephone BH), looks like working, I'm able to 
receive the expected QMI USSD Indication messages.


---
 From 2f041e6ba183a2f2a3309e139adef3d114bdc85b Mon Sep 17 00:00:00 2001
From: Alexey Andreyev 
Date: Fri, 19 Feb 2021 15:47:42 +0300
Subject: [PATCH] qmimodem: Add USSD indication support

Handle USSD QMI indication messages.
Add support for UCS2 USS Data coding scheme.
Check for User Action TLV type.
---
drivers/qmimodem/ussd.c | 45 +
1 file changed, 45 insertions(+)



I had to hand-edit this patch for it to apply and then fix up the whitespace 
formatting.  Please get git send-email working properly for future submissions.


Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH phonesim 6/7] Port to Qt 6

[Denis Kenzior]

Hi Jonah,

On 1/18/21 10:46 AM, Jonah Brüchert wrote:

---
  CMakeLists.txt  | 10 +++---
  src/CMakeLists.txt  |  6 +-
  src/control.cpp |  8 +++-
  src/hardwaremanipulator.cpp |  1 -
  src/phonesim.cpp|  2 +-
  src/qsimcommand.cpp |  2 +-
  src/qwsppdu.cpp | 12 ++--
  7 files changed, 23 insertions(+), 18 deletions(-)



Can the code changes be made standalone, without the build-system bits?

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH phonesim 3/7] Remove autotools build system

[Denis Kenzior]

Hi Jonah,

On 1/18/21 10:46 AM, Jonah Brüchert wrote:

---
  INSTALL | 236 
  Makefile.am |  79 ---
  bootstrap   |   3 -
  bootstrap-configure |   9 --
  configure.ac|  49 -
  5 files changed, 376 deletions(-)
  delete mode 100644 INSTALL
  delete mode 100644 Makefile.am
  delete mode 100755 bootstrap
  delete mode 100755 bootstrap-configure
  delete mode 100644 configure.ac



Since Marcel is the one making releases, I will let him  decide whether this is 
something he wants to do or not.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH phonesim 5/7] qwsppdu: Port QDateTime deprecations

[Denis Kenzior]

Hi Jonah,

On 1/18/21 10:46 AM, Jonah Brüchert wrote:

---
  src/qwsppdu.cpp | 8 
  1 file changed, 8 insertions(+)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH phonesim 2/7] Use quotes to include local header files

[Denis Kenzior]

Hi Jonah,

On 1/18/21 10:46 AM, Jonah Brüchert wrote:

Co-authored-by: Alexander Akulich 
---
  src/aidapplication.cpp|  4 ++--
  src/callmanager.cpp   |  4 ++--
  src/conformancesimapplication.cpp |  2 +-
  src/control.h |  2 +-
  src/hardwaremanipulator.cpp   | 14 +++---
  src/main.cpp  |  3 ++-
  src/phonesim.cpp  |  2 +-
  src/phonesim.h|  2 +-
  src/qatresult.cpp |  2 +-
  src/qatresultparser.cpp   |  6 +++---
  src/qatutils.cpp  |  6 +++---
  src/qcbsmessage.cpp   |  4 ++--
  src/qgsmcodec.cpp |  2 +-
  src/qsimcommand.cpp   |  8 
  src/qsimcontrolevent.cpp  |  4 ++--
  src/qsimenvelope.cpp  |  2 +-
  src/qsimenvelope.h|  2 +-
  src/qsimterminalresponse.cpp  |  4 ++--
  src/qsimterminalresponse.h|  2 +-
  src/qsmsmessage.cpp   |  8 
  src/qsmsmessage_p.h   |  4 ++--
  src/qwsppdu.cpp   |  2 +-
  src/simapplication.cpp|  2 +-
  src/simapplication.h  |  8 
  src/simauth.cpp   |  4 ++--
  src/simfilesystem.cpp |  2 +-
  26 files changed, 53 insertions(+), 52 deletions(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v3 2/3] sim: validate IMS private identity

[Denis Kenzior]

Hi Sergey,

On 1/16/21 1:21 PM, Sergey Matyukevich wrote:

Make sure that IMS private identity is a valid UTF8 string before
setting sim->impi field. Otherwise ofono may crash on dbus assert
when SIM properties are reported via org.ofono.SimManager interface.
---
  src/sim.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)



Patch  2 and 3 applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v3 1/3] simutil: add validate_utf8_tlv

[Denis Kenzior]

Hi Sergey,

On 1/16/21 1:21 PM, Sergey Matyukevich wrote:

Add helper to validate if TLV value is a valid UTF8 string.
Note that both null-terminated and non null-terminated UTF8
strings are considered valid.
---
  src/simutil.c | 14 ++
  src/simutil.h |  1 +
  2 files changed, 15 insertions(+)






+gboolean validate_utf8_tlv(const unsigned char *tlv)
+{
+   int len = tlv[1];
+
+   if (len == 0)
+   return FALSE;
+
+   /* support both null-termiated and non null-terminated UTF8 TLV value */


I tweaked this comment to fix the typo here and make fit to 80 chars


+   if (tlv[len + 1] == '\0')
+   len -= 1;
+
+   return g_utf8_validate_len((const char *)tlv + 2, len, NULL);
+}
+
  static char *sim_network_name_parse(const unsigned char *buffer, int length,
gboolean *add_ci)
  {


Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] sim: validate IMS private identity

[Denis Kenzior]

Hi Sergey,

On 1/15/21 2:34 PM, Sergey Matyukevich wrote:

Make sure that IMS private identity is a valid UTF8 string before
setting sim->impi field. Otherwise ofono may crash on dbus assert
when SIM properties are reported via org.ofono.SimManager interface.
---
   src/sim.c | 3 ++-
   1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/sim.c b/src/sim.c
index 33e1245f..2a663e2d 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1664,7 +1664,8 @@ static void impi_read_cb(int ok, int total_length, int 
record,
return;
}
-   sim->impi = g_strndup((const char *)data + 2, data[1]);
+   if (g_utf8_validate((const char *)data + 2, data[1], NULL))
+   sim->impi = g_strndup((const char *)data + 2, data[1]);


I assume this code path was tested with a file containing embedded NULs as
that is the only way it would have worked.


Ignore the last part of the above sentence.  What I'm trying to say is that:

We in theory have two possibilities:

1. file with a string 'foo', no null:
0x80 0x03 'f' 'o' 'o'

2. file with  a string 'foo' and null:
0x80 0x04 'f' 'o' 'o'

I suspect the spec really wants 1, but maybe it can be interpreted that 2 is 
also a possibility?


The present logic should work for either of the above, but not what you have, 
i.e.:

0x80 0x03 0xff 0xff 0xff



glib docs [1] say:
"Note that g_utf8_validate() returns FALSE if max_len is positive and any of
the max_len bytes are nul."

So I think the above logic would flag such a file as invalid, no?




...but g_utf8_validate as invoked in this patch would flag possibility 2 as 
invalid...



No, I tested using modem with attached SIM/eSIM. TLV data object appears
to be well-formed, but the contents is all padding 0xff bytes. Could you
please clarify your concern ? I assume we can not rely on the content
being properly null terminated string.


Yeah, for the case you have this logic works just fine.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] sim: validate IMS private identity

[Denis Kenzior]

Hi Sergey,

On 1/15/21 1:56 PM, Sergey Matyukevich wrote:

Make sure that IMS private identity is a valid UTF8 string before
setting sim->impi field. Otherwise ofono may crash on dbus assert
when SIM properties are reported via org.ofono.SimManager interface.
---
  src/sim.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/src/sim.c b/src/sim.c
index 33e1245f..2a663e2d 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -1664,7 +1664,8 @@ static void impi_read_cb(int ok, int total_length, int 
record,
return;
}
  
-	sim->impi = g_strndup((const char *)data + 2, data[1]);

+   if (g_utf8_validate((const char *)data + 2, data[1], NULL))
+   sim->impi = g_strndup((const char *)data + 2, data[1]);


I assume this code path was tested with a file containing embedded NULs as that 
is the only way it would have worked.


glib docs [1] say:
"Note that g_utf8_validate() returns FALSE if max_len is positive and any of the 
max_len bytes are nul."


So I think the above logic would flag such a file as invalid, no?


  }
  
  static void discover_apps_cb(const struct ofono_error *error,

___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org



Regards,
-Denis

[1] 
https://developer.gnome.org/glib/stable/glib-Unicode-Manipulation.html#g-utf8-validate

___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] sim: validate IMS private identity

[Denis Kenzior]

Hi Sergey,


This field may not be defined for ISIM in use. In this case the
field still can be read from ISIM, though it will not contain
a valid UTF8 string. For instance, it may contain 255 0xFF bytes.


Ugh, seems like the SIM vendor can't follow RFC's either?  31.103 Section
4.2.2 says:

"For contents and syntax of NAI TLV data object values see IETF RFC 2486
[24]. The NAI shall be encoded to an octet string according to UTF-8
encoding rules as specified in IETF RFC 3629 [27]. The tag value of the NAI
TLV data object shall be '80'. "


This crash occured during my experiments with eSIM. As I mentioned, the
content of that TLV data object was 0xff bytes. IIUC it looks like vendor
could just skip initialization of that particular TLV data object during
bootstrap. Though I am not yet familiar with eSIM init procedure...



I figured.  The likely reason for this is that SIM strings are generally encoded 
in another format.  If you're interested, see src/util.c sim_string_to_utf8(). 
0xff is used as padding in such cases and thus a field with only 0xffs is 
treated as empty.


However, the  above would seem not to apply to EFimpi and a few others that 3GPP 
wants encoded as a utf-8 string.  So, likely, a bug on the SIM, but we should 
have sanitized the contents better.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] sim: validate IMS private identity

[Denis Kenzior]

Hi Sergey,

On 1/15/21 10:38 AM, Sergey Matyukevich wrote:

Make sure that IMPI is a valid UTF8 string before attempting
to report it via DBus. Otherwise ofono may crash on dbus assert.
This field may not be defined for ISIM in use. In this case the
field still can be read from ISIM, though it will not contain
a valid UTF8 string. For instance, it may contain 255 0xFF bytes.


Ugh, seems like the SIM vendor can't follow RFC's either?  31.103 Section 4.2.2 
says:


"For contents and syntax of NAI TLV data object values see IETF RFC 2486 [24]. 
The NAI shall be encoded to an octet string according to UTF-8 encoding rules as 
specified in IETF RFC 3629 [27]. The tag value of the NAI TLV data object shall 
be '80'. "



---
  src/sim.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/sim.c b/src/sim.c
index 33e1245f..f60f5d1b 100644
--- a/src/sim.c
+++ b/src/sim.c
@@ -423,7 +423,7 @@ static DBusMessage *sim_get_properties(DBusConnection *conn,
ofono_dbus_dict_append(, "ServiceProviderName",
DBUS_TYPE_STRING, >spn);
  
-	if (sim->impi)

+   if (sim->impi && g_utf8_validate(sim->impi, 255, NULL))


Hmm, this would imply that we're setting sim->impi incorrectly..  Also, since we 
have __ofono_sim_get_impi() API, the better fix would be to make sure sim->impi 
is set correctly in impi_read_cb()



ofono_dbus_dict_append(, "ImsPrivateIdentity",
DBUS_TYPE_STRING, >impi);
  
___

ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2 3/3] gemalto: netmon measurements scaling

[Denis Kenzior]

Hi Sergey,

On 1/15/21 10:25 AM, Sergey Matyukevich wrote:

Gemalto modem reports raw measurements in dBm. Reported values may
include negative numbers. Meanwhile oFono follows ETSI TS 27.007,
so negative numbers do not really exist at the API level.

Modify gemalto netmon driver to report measurements according to
27.007. For this purpose re-scale from what Gemalto firmware
reports into something that 27.007 recommends.
---
  drivers/gemaltomodem/netmon.c | 52 ---
  1 file changed, 48 insertions(+), 4 deletions(-)



I went ahead and applied all three patches after making minor tweaks in this 
one:


+static int gemalto_rscp_scale(int value)
+{
+   if (value < -120)
+   return 0;
+
+   if (value > -24)
+   return 96;
+
+   return (value + 120);


The ()s weren't needed here..


+}
+
+static int gemalto_rsrp_scale(int value)
+{
+   if (value < -140)
+   return 0;
+
+   if (value > -43)
+   return 97;
+
+   return (value + 140);


here and ...


+}
+
+static int gemalto_rsrq_scale(int value)
+{
+   if (2 * value < -39)
+   return 0;
+
+   if (2 * value > -5)
+   return 34;
+
+   return (2 * value + 39);


here


+}
+
  static int gemalto_parse_smoni_gsm(GAtResultIter *iter,
struct req_cb_data *cbd)
  {
@@ -273,13 +317,13 @@ static int gemalto_parse_smoni_umts(GAtResultIter *iter,
case SMONI_UMTS_ECN0:
if (g_at_result_iter_next_unquoted_string(iter, )) {
if (sscanf(str, "%f", ) == 1)
-   cbd->t.umts.ecno = (int)fnumber;
+   cbd->t.umts.ecno = 
gemalto_ecno_scale((int)fnumber);


Also added a line break to better wrap to 80 char columns here...


}
break;
case SMONI_UMTS_RSCP:
if (g_at_result_iter_next_unquoted_string(iter, )) {
if (sscanf(str, "%d", ) == 1)
-   cbd->t.umts.rscp = number;
+   cbd->t.umts.rscp = 
gemalto_rscp_scale(number);


and here


}
break;
case SMONI_UMTS_MCC:


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [RFC PATCH 0/2] gemalto: netmon driver

[Denis Kenzior]

Hi Sergey,

On 1/10/21 1:44 PM, Sergey Matyukevich wrote:

Hello Denis and all,

This patch series implements netmon driver for gemalto modems that are
able to provide basic measurements using AT+CQS and AT^SMONI commands.

This patch series is intendedly marked as RFC. In addition to general


I guess you mean intentionally here...


feedback for this v1 I would like to clarify the right way to handle
negative values that can be returned by modem. For instance, this


So the short answer is: You're not supposed to even have negative numbers in AT 
commands.  And since oFono follows 27.007, which is itself based on AT commands, 
negative numbers do not really exist at the API level.


Refer to ITU v.250 Section 5.4.2.1 "Numeric constants".  Strings are also 
supposed to be formatted in a certain way, which the firmware isn't doing 
properly either...  The fact that you have to resort to using 
next_unquoted_string is kind of telling.



is the case for the values measured in dBm including EC/n0 and RSCP.
Currently such values are discarded by CELL_INFO_DICT_APPEND macro.


I would guess your best bet would be to re-scale from what Gemalto firmware 
reports into something that 27.007 recommends.  You may have to refer to 27.007, 
Section 8.69 to see what the scale is according to 3GPP.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [RFC PATCH 1/2] gemalto: add netmon driver

[Denis Kenzior]

Hi Sergey,

On 1/10/21 1:44 PM, Sergey Matyukevich wrote:

Implement network monitoring driver for gemalto modems that
are able to provide serving cell information and basic
measurements using AT+CQS and AT^SMONI commands.
---
  Makefile.am |   3 +-
  drivers/gemaltomodem/gemaltomodem.c |   2 +
  drivers/gemaltomodem/gemaltomodem.h |   3 +
  drivers/gemaltomodem/netmon.c   | 609 
  4 files changed, 616 insertions(+), 1 deletion(-)
  create mode 100644 drivers/gemaltomodem/netmon.c






+static int gemalto_netmon_probe(struct ofono_netmon *netmon,
+   unsigned int vendor, void *user)
+{
+   GAtChat *chat = user;
+   struct netmon_driver_data *nmd;
+
+   DBG("gemalto netmon probe");
+
+   nmd = g_try_new0(struct netmon_driver_data, 1);
+   if (nmd == NULL)
+   return -ENOMEM;


Feel free to use g_new0.  We have given up using g_try_new0 since these errors 
don't really occur on Linux.



+
+   nmd->chat = g_at_chat_clone(chat);
+
+   ofono_netmon_set_data(netmon, nmd);
+
+   g_idle_add(gemalto_delayed_register, netmon);
+
+   return 0;
+}


Other than this, LGTM.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] quectel: adding support for the Quectel EC200 USB modem series

[Denis Kenzior]

Hi Sergei,

On 1/7/21 2:40 PM, s.e.golubt...@gmail.com wrote:

From: Sergei Golubtsov 

Support for the Quectel EC200 USB modem series has been added. The model
identification AT command has been added as the first step in the
communication with a Quectel USB modem.
---
  plugins/quectel.c | 128 +-
  plugins/udevng.c  |   6 ++-
  2 files changed, 85 insertions(+), 49 deletions(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] ppp: using RX ACCM = 0 by default

[Denis Kenzior]

Hi Sergei,

On 1/7/21 2:40 PM, s.e.golubt...@gmail.com wrote:

From: Sergei Golubtsov 

Some modems such as Quectel EC200T do not honor the default value for
the Async-Control-Character-Map (ACCM) configuration option defined in
RFC 1548 6.2 as 0x. This patch suggests to use RX ACCM = 0 for
Ofono by default as pppd does for instance. This will reduce PPP data
overhead as well.
---
  gatchat/gatppp.c  |  5 +
  gatchat/gatppp.h  |  1 +
  gatchat/ppp.h |  1 +
  gatchat/ppp_lcp.c | 15 ++-
  4 files changed, 21 insertions(+), 1 deletion(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] gsmdial: adding support for selection of authentication method

[Denis Kenzior]

Hi Sergei,

On 1/7/21 2:40 PM, s.e.golubt...@gmail.com wrote:

From: Sergei Golubtsov 

Selection capability for authentication method via a command line
argument has been added
---
  gatchat/gsmdial.c | 16 
  1 file changed, 16 insertions(+)






@@ -369,6 +370,17 @@ static void connect_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
}
g_at_ppp_set_debug(ppp, gsmdial_debug, "PPP");
  
+	GAtPPPAuthMethod auth_method;

+
+   if (strcmp(option_auth_method, "PAP") == 0)


note that option_auth_method might be NULL and since you're using strcmp, 
passing NULL might result in undefined behavior.



+   auth_method = G_AT_PPP_AUTH_METHOD_PAP;
+   else if (strcmp(option_auth_method, "NONE") == 0)
+   auth_method = G_AT_PPP_AUTH_METHOD_NONE;
+   else
+   auth_method = G_AT_PPP_AUTH_METHOD_CHAP;
+
+   g_at_ppp_set_auth_method(ppp, auth_method);
+
g_at_ppp_set_credentials(ppp, option_username, option_password);
  
  	g_at_ppp_set_acfc_enabled(ppp, option_acfc);


Note that I still get the following compiler error:

denkenz@localhost ~/ofono-master $ make
make --no-print-directory all-am
  CC   gatchat/gsmdial.o
gatchat/gsmdial.c: In function ‘connect_cb’:
gatchat/gsmdial.c:373:2: error: ISO C90 forbids mixed declarations and code 
[-Werror=declaration-after-statement]

  373 |  GAtPPPAuthMethod auth_method;
  |  ^~~~

oFono code must compile with gnu89 dialect of GCC, so all variable declarations 
must be in the appropriate block.  Please make sure you compile test with 
optimization enabled in order for gcc to catch any errors/warnings.


I went ahead and modified the patch slightly and applied it to take care of 
this.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,


So far, all modems on this planet have honored the default properly...

Sorry for my misunderstanding. I should have read the RFC better but this aspect 
is defined

properly only in the following clause -
"TheConfiguration Option is used to inform the peer which control

characters MUST remain mapped when the peer sends them."

So I have managed to find this after your message. Thank you.

So you have two options for fixing this:
    - Make GAtPPP negotiate a default ACCM in all cases

    - Override the negotiated ACCM (using g_at_hdlc_set_recv_accm for your
particular modem.

I think that this modem is buggy as you have clearly shown. I agree that it is 
not a good idea to
do something which does not follow the RFCs fine. But, I feel that the default 


I don't remember why we chose not to negotiate the RX ACCM.  Things may have 
worked wonderfully with the defaults, so maybe we just decided not to touch it.



value for ACCM
is not that one we should use. Excuse me if I am too direct here, I am not an 
expert in this field
but I think it is better for DTE to send a configuration request with ACCM = 
0x
as pppd does because this allows us to send more information in the same number 
of bytes.
Would you agree? If not I will override the negotiated ACCM, if it is left 


I think that makes sense.


default, for this
chinese modem because I do not feel that they could update the firmware to 
fulfill the RFC
requirements in a reasonable amount of time if they even would. I do not think 
that they

would even find this necessary if I had told them.


Well, it isn't a 'requirement' to use the defaults.  What happened in this 
particular case is that we didn't explicitly tell the modem side what RX ACCM we 
want, and the modem firmware didn't use the defined default values.


I think it should be safe for us to use an ACCM of 0 instead of the RFC default. 
 But just in case, we should probably provide a way for the driver to override 
it if needed.


So what I would do is:

- Set REQ_OPTION_ACCM in the lcp init somewhere.  Set the initial value to be 0 
as you suggest.
- Add g_at_ppp_set_accm() in order to allow drivers to provide a custom ACCM to 
negotiate, if needed.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,

On 1/2/21 3:52 AM, Sergei Golubtsov wrote:

Hi Denis,



Happy New Year.  Apologies for the late response, I was in holiday mode.


The log from my previous message clearly shows that the problem is that the
modem stopped escaping control characters (<0x20) with 0x7d when phase 2 had
begun. Apparently pppd is ok with this behaviour. Am going to compare the
HLDC frame parsers in pppd and ofono. Is it a modem bug or an ofono missing
feature? Any help will be appreciated.

I have found the problem. ofono does not apply ACCM, which is proposed by the 


So you're on the right track, but I think you're coming to the wrong conclusion. 
 Things are a bit more subtle than that.


DCE in a configuration request. The EC200 Quectel modem uses ACCM = 0x 
in the authentication phase and ofono ACK such configuration but does not apply 
configuration options. That is why ofono silently drops some frames from the DCE 


No, this is not correct.   oFono does honor the ACCM sent by the modem.  This 
happens in lcp_rcr().   Look around ppp_lcp.c:321 or so.  We apply the options 
just prior to sending the ack.


Remember, there are two ACCMs, the transmit and the receive.  Refer to RFC 1662, 
Section 7.1:


"Each end of the asynchronous link maintains two Async-Control-Character-Maps. 
The receiving ACCM is 32 bits, but the sending ACCM may be up to 256 bits.  This 
results in four distinct ACCMs, two in each direction of the link."


The receive ACCM is the issue here.  RFC 1662 also says:
"For asynchronous links, the default receiving ACCM is 0x.  The default 
sending ACCM is 0x, plus the Control Escape  and Flag Sequence 
characters themselves, plus whatever other outgoing characters are flagged (by 
prior configuration) as likely to be intercepted."


GAtPPP basically doesn't want or need to re-negotiate the default ACCM, hence we 
do not send an ACCM to the peer, expecting it to use the defaults.  See 
lcp_generate_config_options().  REQ_OPTION_ACCM is never set.  What this means 
is that when the peer acks our options and we call lcp_rca in 
pppcp_process_configure_ack(), the ACCM option is not present.  Hence the code 
for setting the ACCM in lcp_rca() is never triggered.


So I suspect what is happening is that your Quectel modem assumes that not 
requesting the ACCM means having a zero ACCM instead of the RFC-mandated default.


So far, all modems on this planet have honored the default properly...

in phase 2 and does not answer to packets because the FCS (CRC) of packets does 
not match the value calculated by ofono. I have sent a patch, which solves the 
problem.

I am concerned about two things:
1. Did I choose the right place to insert the configuration options application 
code?


So you have two options for fixing this:
  - Make GAtPPP negotiate a default ACCM in all cases
  - Override the negotiated ACCM (using g_at_hdlc_set_recv_accm for your 
particular modem.


2. ACCM is for asynchronous links only. Does ofono support other links? If so we 
must check the link type before applying the ACCM configuration option as 
suggested in RFC 1812  3.3.5.2.




We only support PPP over serial ports or emulated serial ports, which have to 
use ACCM.  I'm not aware of any other possibilities.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH ppp] ppp: Apply configuration options from a DCE request if ACK it

[Denis Kenzior]

Hi Sergei,

On 1/1/21 3:04 PM, s.e.golubt...@gmail.com wrote:

From: Sergei Golubtsov 

Apply configuration options if the DCE sends a PPP LCP configure request
and the DTE is going to ACK the request. Otherwise some options such as
the ACCM may not correspond to the configuration negotiated with the
DCE.
---
  gatchat/ppp_cp.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/gatchat/ppp_cp.c b/gatchat/ppp_cp.c
index f3f2cc4f..371055e4 100644
--- a/gatchat/ppp_cp.c
+++ b/gatchat/ppp_cp.c
@@ -660,9 +660,12 @@ static void pppcp_generate_event(struct pppcp_data *data,
if (actions & SCR)
pppcp_send_configure_request(data);
  
-	if (actions & SCA)

+   if (actions & SCA) {
+   /* Apply local options */
+   if (data->driver->rca)
+   data->driver->rca(data, packet);


So this is incorrect.  You're applying options of the wrong 'side'.  See my 
reply in the other thread for  a more detailed explanation.



pppcp_send_configure_ack(data, packet);
-   else if (actions & SCN)
+   } else if (actions & SCN)
pppcp_send_configure_nak(data, packet);
  
  	if (actions & STR)




Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] gsmdial: add support for different authentication methods selection

[Denis Kenzior]

Hi Sergei,

On 1/1/21 3:12 PM, s.e.golubt...@gmail.com wrote:

From: Sergei Golubtsov 

Selection capability for authentication method via a command line
argument has been added
---
  gatchat/gsmdial.c | 13 +
  1 file changed, 13 insertions(+)



denkenz@localhost ~/ofono-master $ make
make --no-print-directory all-am
  CC   gatchat/gsmdial.o
gatchat/gsmdial.c: In function ‘connect_cb’:
gatchat/gsmdial.c:373:2: error: ISO C90 forbids mixed declarations and code 
[-Werror=declaration-after-statement]

  373 |  GAtPPPAuthMethod auth_method = G_AT_PPP_AUTH_METHOD_CHAP;
  |  ^~~~
cc1: all warnings being treated as errors
make[1]: *** [Makefile:4078: gatchat/gsmdial.o] Error 1
make: *** [Makefile:2376: all] Error 2



diff --git a/gatchat/gsmdial.c b/gatchat/gsmdial.c
index 60e4f245..0407f97c 100644
--- a/gatchat/gsmdial.c
+++ b/gatchat/gsmdial.c
@@ -53,6 +53,7 @@ static gint option_cid = 0;
  static gchar *option_apn = NULL;
  static gint option_offmode = 0;
  static gboolean option_legacy = FALSE;
+static gchar *option_auth_method;
  static gchar *option_username = NULL;
  static gchar *option_password = NULL;
  static gchar *option_pppdump = NULL;
@@ -369,6 +370,14 @@ static void connect_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
}
g_at_ppp_set_debug(ppp, gsmdial_debug, "PPP");
  
+	GAtPPPAuthMethod auth_method = G_AT_PPP_AUTH_METHOD_CHAP;

+   if (strcmp(option_auth_method, "PAP") == 0) {
+   auth_method = G_AT_PPP_AUTH_METHOD_PAP;
+   } else if (strcmp(option_auth_method, "NONE") == 0) {
+   auth_method = G_AT_PPP_AUTH_METHOD_NONE;
+   }
+   g_at_ppp_set_auth_method(ppp, auth_method);
+


Also please refer to doc/coding-style.txt item M1.


g_at_ppp_set_credentials(ppp, option_username, option_password);
  
  	g_at_ppp_set_acfc_enabled(ppp, option_acfc);

@@ -677,6 +686,10 @@ static GOptionEntry options[] = {
"Use ATD*99***#" },
{ "bluetooth", 'b', 0, G_OPTION_ARG_NONE, _bluetooth,
"Use only ATD*99" },
+   { "auth", 'A', 0, G_OPTION_ARG_STRING, _auth_method,
+   "Specify the authentication method for the PPP"
+   " connection: CHAP, PAP or NONE. CHAP is used"
+   " by default." },
{ "username", 'u', 0, G_OPTION_ARG_STRING, _username,
"Specify PPP username" },
{ "password", 'w', 0, G_OPTION_ARG_STRING, _password,



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] quectel: add support of Quectel EC200 USB modem

[Denis Kenzior]

Hi Sergei,

On 1/1/21 3:17 PM, s.e.golubt...@gmail.com wrote:

From: Sergei Golubtsov 

Support of Quectel EC200 USB modem series has been added. The model
identification AT command has been added as the first step in the
communication with a Quectel modem.
---
  plugins/quectel.c | 49 +++
  plugins/udevng.c  |  6 --
  2 files changed, 49 insertions(+), 6 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 82fc688d..d77fbe60 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -64,7 +64,7 @@ static const char *cpin_prefix[] = { "+CPIN:", NULL };
  static const char *cbc_prefix[] = { "+CBC:", NULL };
  static const char *qinistat_prefix[] = { "+QINISTAT:", NULL };
  static const char *cgmm_prefix[] = { "UC15", "Quectel_M95", "Quectel_MC60",
-   "EC21", NULL };
+   "EC21", "EC200", NULL };
  static const char *none_prefix[] = { NULL };
  
  static const uint8_t gsm0710_terminate[] = {

@@ -84,6 +84,7 @@ enum quectel_model {
QUECTEL_M95,
QUECTEL_MC60,
QUECTEL_EC21,
+   QUECTEL_EC200,
  };
  
  struct quectel_data {

@@ -127,6 +128,15 @@ enum quectel_power_event {
  
  static const char dbus_hw_interface[] = OFONO_SERVICE ".quectel.Hardware";
  
+static void cgmm_cb(int ok, GAtResult *result, void *user_data);


Please see doc/coding-style.txt item M17


+
+static ofono_bool_t has_serial_connection(struct ofono_modem *modem)
+{
+   if (ofono_modem_get_string(modem, "Device"))
+   return TRUE;
+   return FALSE;


doc/coding-style.txt item M1


+}
+
  static void quectel_debug(const char *str, void *user_data)
  {
const char *prefix = user_data;
@@ -543,6 +553,7 @@ static void dbus_hw_enable(struct ofono_modem *modem)
switch (data->model) {
case QUECTEL_UC15:
case QUECTEL_EC21:
+   case QUECTEL_EC200:
g_at_chat_register(data->aux, "+QIND",  qind_notify, FALSE, hw,
NULL);
break;
@@ -591,6 +602,13 @@ static void qinistat_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
/* UC15 uses a bitmap of 1 + 2 + 4 = 7 */
ready = 7;
break;
+   case QUECTEL_EC200:
+   /*
+* EC200T doesn't indicate that the Phonebook initialization
+* is completed (==4) when AT+CFUN=4, that's why 1 + 2 = 3
+*/
+   ready = 3;
+   break;
case QUECTEL_M95:
case QUECTEL_MC60:
/* M95 and MC60 uses a counter to 3 */
@@ -794,6 +812,18 @@ static void cfun_query(gboolean ok, GAtResult *result, 
gpointer user_data)
cfun_enable(TRUE, NULL, modem);
  }
  
+static void identify_model(struct ofono_modem *modem)

+{
+   struct quectel_data *data = ofono_modem_get_data(modem);
+
+   DBG("%p", modem);
+
+   g_at_chat_set_slave(data->modem, data->aux);


Why do you need this here?  This really belongs in open_ttys as opposed to 
setup_aux or here.

+
+   g_at_chat_send(data->aux, "AT+CGMM", cgmm_prefix, cgmm_cb, modem,
+   NULL);
+}
+
  static void setup_aux(struct ofono_modem *modem)
  {
struct quectel_data *data = ofono_modem_get_data(modem);
@@ -807,6 +837,9 @@ static void setup_aux(struct ofono_modem *modem)
NULL, NULL, NULL);
g_at_chat_send(data->aux, "AT+QURCCFG=\"urcport\",\"uart1\"", 
none_prefix,
NULL, NULL, NULL);
+   } else if (data->model == QUECTEL_EC200) {
+   g_at_chat_send(data->aux, "ATE0;  +CMEE=1", none_prefix,
+   NULL, NULL, NULL);
} else
g_at_chat_send(data->aux, "ATE0;  +CMEE=1; +QIURC=0",
none_prefix, NULL, NULL, NULL);
@@ -834,7 +867,7 @@ static int open_ttys(struct ofono_modem *modem)
return -EIO;
}
  
-	setup_aux(modem);

+   identify_model(modem);


This results in Serial versions of Quectel modems issuing CGMM twice.  The logic 
also becomes very hard to follow.


Perhaps CGMM needs to be issued once after open_ttys (like you're doing for USB 
variants) for both Serial and USB versions.


  
  	return -EINPROGRESS;

  }
@@ -1061,14 +1094,22 @@ static void cgmm_cb(int ok, GAtResult *result, void 
*user_data)
DBG("%p model EC21", modem);
data->vendor = OFONO_VENDOR_QUECTEL_EC2X;
data->model = QUECTEL_EC21;
+   } else if (strstr(model, "EC200")) {
+   DBG("%p model %s", modem, model);
+   data->vendor = OFONO_VENDOR_QUECTEL_EC2X;
+   data->model = QUECTEL_EC200;
} else {
ofono_warn("%p unknown model: '%s'", modem, model);
data->vendor = OFONO_VENDOR_QUECTEL;

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,

I still cannot understand why ofono does not try to use PAP if the 
authentication method is set to CHAP by default (because no method set while 
context creation) and it is "obvious" that PAP should be used. pppd does this 
BTW, that's why I assumed that it could be "obvious". I am not an expert in this 
field. Maybe someone could shed a light on the fact that pppd chooses a correct 
authentication method without a problem and ofono does not?


It has been a very long time since we developed PPP (about 10 years now), so my 
memory may be fuzzy now.  But the answer to your question is that it isn't 
possible to auto-detect this.


The issue is, that the 'PPP server' running in the firmware has no clue what the 
authentication should be.  It is literally taking auth frames from the client 
and forwarding them to the network.  The network is the one that actually knows 
what the authentication details are.  3GPP mandates that CHAP is the 'default' 
method for such cases.  But seems some providers had legacy software that 
demanded PAP, and instead of fixing the backend they pushed all these details 
out to the front-end.  Hence why you have a need of provisioning data bases like 
'mobile-broadband-provider-info' or the android APN db, etc.


So, the LCP configuration stage (where the auth options are chosen) is performed 
prior to any of the frames being exchanged between the modem and the network. 
You can see this in your log 'ppp_dump_single_port.txt', where gsmdial is 
configured for CHAP, NAKs the PAP auth proto and the modem firmware happily 
switches to CHAP.  Why the subsequent CHALLENGE messages from the modem go 
unanswered by gsmdial is not clear to me.




Am I correct that my SW has to know which authentication must be used before it 
attempts to activate context for the first time? This is a big inconvenience. 
Actually the fact that the context must be activated manually at the first time 
is the big inconvenience too for embedded systems. But this is not the biggest 
issue for me right now of course.


Unfortunately yes.  Look into provisioning contexts automatically.  See 
plugin/provision.c,  plugins/file-provision.c




 > Never tried this myself, but I never really had a reason to.  Not sure if 
there
 > is an easy way of doing that given you're running over a MUX.  If you come up
 > with a nice way, please share.

Sorry, I have not got your point about MUX. I have successfully obtained the 
dumps. I have 2 ttyUSB devices and gsmdial works fine with one tty as well as 
with both. Could you elaborate a little bit more on the MUX issue?


Didn't you say you have a Quectel  device?  Some of those are a serial port 
based modem and require a multiplexer.  I guess you have the USB variant so what 
I said doesn't apply.




Please find the dumps attached. I did not find any problems in them. DTE sent a 
termination request but why? I tried to use the CHAP, PAP and NONE auth 
metnods but NONE hands with gsm dial.


I don't see pppd getting any IPCP protocol going, so I'm not really sure any of 
this is working.



I am going to debug ofono as you suggested:

 > Maybe put a few debug statements in ppp_auth.c, ppp_pap_start, 
ppp_pap_timeout
 > and ppp_pap_process_packet.  See if PAP even occurs.

Also I have made a small patch for gsmdial that helps to choose auth method. 
Please find it attached.


Feel free to send this in using git send-email.  I don't review patches in 
attachments.  Note that the last time I looked at gsmdial was ~9 years ago, so 
not sure how up to date it is compared to the ppp based gprs context code in 
drivers/atmodem.  You may want to use g_at_ppp_set_recording (maybe via 
environment variable) from drivers/atmodem/gprs-context.c directly in order to 
obtain dumps of what is happening when oFono is attempting to activate the context.


I use cf9e6d048d65ff4a87f5707b0cc4fd3c036d62fb in my patch because the last 
commit in master de0d5a19c16bbb3eaba473e0eaaade7f55c48bcb  does not build for me 
because of  goto was left in a function without any label. Please see the error 
below and in the file attached:
| ../git/ofono/drivers/gemaltomodem/gprs-context.c: In function 
'gemalto_gprs_activate_primary':
| ../git/ofono/drivers/gemaltomodem/gprs-context.c:156:3: error: label 'error' 
used but not defined

|    goto error;
|    ^~~~



Please git pull --rebase.  I fixed this several days ago and there have been 
other commits applied since.


Maybe I should add some "properties" to context for this modem before 
activation?  Also I am not happy with the idea that I probably need to set an 
authentication method by hand. Could this really be necessary with ofono in some 
situations? I added the pppd configuration files of the modem vendor which work 
fine. Do I need to add something to the context in ofono before activation?


See above about provisioning.



I strongly believe that ofono as embedded system software has to be as automated 
as possible. Do you 

Re: [PATCH v2 0/2] gemalto: gprs context driver updates

[Denis Kenzior]

Hi Sergey,

On 12/26/20 2:56 PM, Sergey Matyukevich wrote:

Hi Denis and all,

This is the second version of the patches updating gemalto gprs context
driver. Major changes include support for automatic context activation
and support for gprs context authentication settings.

Regards,
Sergey

v1 -> v2

- drop patches 1, 2, 4 that has been already applied
- add detailed comment on AT^SWWAN patch
- address review comments


Sergey Matyukevich (2):
   gemalto: gprs: support automatic context activation
   gemalto: gprs: support authentication settings

  drivers/gemaltomodem/gprs-context.c | 155 
  1 file changed, 114 insertions(+), 41 deletions(-)



All applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,

On 12/30/20 4:00 AM, Sergei Golubtsov wrote:

Hello Denis,

 > Are the username and password set properly on the context?

Yes, they are.
root@131D2F4C5254324B:~# /usr/lib/ofono/test/list-contexts
[ /quectel_0 ]
     [ /quectel_0/context1 ]
         Name = Россия
         Active = 0
         Type = internet
         Protocol = ip
         AccessPointName = internet
         Username = gdata
         Password = gdata
         AuthenticationMethod = pap
         Settings = { }
         IPv6.Settings = { }

     [ /quectel_0/context2 ]
         Name = МегаФон MMS
         Active = 0
         Type = mms
         Protocol = ip
         AccessPointName = mms
         Username = mms
         Password = mms
         AuthenticationMethod = pap
         MessageProxy = 10.10.10.10:8080 
         MessageCenter = http://mmsc:8002
         Settings = { }
         IPv6.Settings = { }



Looks fine...

 > And you may want to share the log of oFono activating the context configured 
for PAP.


I have added the PAP context activation log file from ofono to this email as 
well as I did in the previous email. Please find it attached. It also fails at 


It wasn't clear whether the previous log was pap or chap.  With chap the PPP 
implementation would stop and wait for the chap challenge.  So if that never 
comes, that would explain why you get stuck just after entering phase '2' -> 
PHASE_AUTHENTICATION



the same phase: "gatppp.c:ppp_enter_phase() 2"
Probably I need to try to dump the ppp connection and analyze it with WireShark. 
Any ideas will be appreciated.


Never tried this myself, but I never really had a reason to.  Not sure if there 
is an easy way of doing that given you're running over a MUX.  If you come up 
with a nice way, please share.


Maybe put a few debug statements in ppp_auth.c, ppp_pap_start, ppp_pap_timeout 
and ppp_pap_process_packet.  See if PAP even occurs.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: Quectel EC200T USB: Problems with context activation

[Denis Kenzior]

Hi Sergei,

On 12/29/20 11:56 AM, Sergei Golubtsov wrote:

Changing the authentication method to pap did not help. Please help.



Are the username and password set properly on the context?  And you may want to 
share the log of oFono activating the context configured for PAP.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [pulseaudio-discuss] Proposal for a new API and usage of Bluetooth HSP and HFP profiles on Linux

[Denis Kenzior]

Hi Pali,




I think you will need to do this regardless.  Otherwise I fail to see how
you prevent one 'agent' from consuming AT commands it shouldn't be. This is
a possibility you need to consider, whether it happens by accident or
maliciously.


Some subset of AT commands are needed to parse and interpret. But not
telephony commands and having up-to-date internal telephony state.


Please think some more about what I said.  You will need to parse every 
AT command in your daemon, no way around that.  You are right that you 
do not need to keep track of the telephony state, but that is besides 
the point.  So if you need an AT parser anyway, the whole reasoning for 
the proposed architecture starts to look shaky.







- The other example is more practical. HFP Service Level Connection (SLC)
establishment is actually quite tricky.  There are certain limitations on
what can and cannot be done prior to SLC establishment, including how audio
handling is done.


I know :-) I already implemented prototype implementation to check and
see how complicated it is and if API make sense and how hard it is for
agents (audio - pulseaudio) implement and maintain it.


Unfortunately, codec negotiation, indicator negotiation,
and feature negotiation are part of the SLC. oFono already solves all of
this and handles all of it nicely.


CSR codecs are not supported nor implemented in ofono. It is more
complicated as HFP codecs... and needs a new API for audio application.
Another value which brings my hsphfpd is that it handles these CSR
codecs and provide API for audio application to use them.



Again, you're not addressing my main point.  Codec negotiation is part 
of SLC establishment.  SLC has both telephony and audio aspects. 
They're inseparable.  Your architecture fails to address this...


CSR codecs are not part of SLC and can be bolted on later.  I already 
told you that oFono can easily be changed to support this.



We have passed all relevant
certification testing.  It is very unclear how you plan to handle this (or
whether you realistically even can) in your architecture when the
responsibilities are split between the various daemons.  So again, oFono has
nothing to gain here...


I was not thinking about certification. It is not something which I
could do And also pulseaudio itself do not have certifications.


So again, no reason for us to get involved :)

Bottom line is there's no value for us in this architecture.  If you 
want to use the existing oFono APIs, that's fine.  But we're not adding 
a plugin for taking arbitrary AT commands from some other daemon :)





Perhaps this can even be solved in oFono itself (since it already does 90%
of what you want) by making the modem requirement optional.  What we could
do for example is to create a dummy modem if an AG connection is requested
and no other suitable modems are detected in the system.  The resultant AG
wouldn't have any call control capability, it could still be used for
transferring audio data, battery, etc.  If you want to pursue this, we can
brainstorm further.


Well, if this would work automatically without any user interaction or
without special setup, it seems to be usable.

But what is needed from this implementation in ofono? Basically API for
each functionality designed in hsphfod daemon. And one of it is also
support for HSP profile (with CSR and Apple extensions).


Start a separate thread on ofono for this.  I already gave you hints on 
how to solve the 'AG without a real modem' use case.  That would seem to 
be the biggest 'win' and it should be fairly easy to make this work.




I'm not against for it, but I thought that having functionality which is
not related to telephony / modem you would not want to see in ofono
project (like linux uinput layer for button events or API for displaying
raw text on embedded display; or CSR audio codec negotiation).

So how do you see possibility to have also HSP profile in ofono? So have
one place which would provide audio API for SCO? Because this is a big
requirements from audio software side, to not use 4 different APIs to
access SCO sockets (and its rfcomm / SLC configuration) in HSP and HFP
profiles.



HSP is a separate issue.  Maybe we can handle it like the 'dundee' 
daemon inside oFono (which handles Bluetooth DUN profile).  In other 
words have a dedicated daemon for hsp support that reuses the relevant 
bits of oFono and maybe exposes the same APIs (i.e the ones documented 
in doc/handsfree-audio-api.txt).  That would make life for PulseAudio 
pretty easy.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [pulseaudio-discuss] Proposal for a new API and usage of Bluetooth HSP and HFP profiles on Linux

[Denis Kenzior]

Hi Pali,


There have been one or two implementations of AG role fully external to
oFono.  These implementations simply used the existing oFono APIs to drive
the modem.


bluez & pulseaudio developers told me that it would be a good idea to
avoid implementing a new AT parser for telephony stack. And rather use
ofono implementation for telephony part...


In my opinion there's nothing scary about AT parsing.  In fact that is 
the easiest part of this whole venture.  If you don't want to roll your 
own parser, you can borrow one from the oFono project.  We already 
solved this nicely in the form of the gatchat library.  You could 
incorporate this into your project (if it is GPL compatible).  Or you 
could wait until we port gatchat to ell which will be under LGPL license.




But if I should use existing DBus API provided by ofono, it means that I
need to do parsing of all AT commands (including telephony) and
translate them to ofono DBus API.


I think you will need to do this regardless.  Otherwise I fail to see 
how you prevent one 'agent' from consuming AT commands it shouldn't be. 
This is a possibility you need to consider, whether it happens by 
accident or maliciously.




I agree, this should work and there is not need to modify ofono. But it
means that in hsphfpd daemon I need to have full AT parser for all
telephony commands and it was something which bluez / pa developers
thought that should be avoided. Therefore I come up with idea that
telephony commands would be handled by external Telephony Agent, which
can be ofono.


Understood.  But I think the metric function was selected 
inappropriately in this case.  My opinion is that you should have 
started with what the overall architecture should look like; you should 
not have based design decisions on which parts might be a little hard to 
implement.





You could do that.  But as I said, we rejected such a design a
long time ago due to complexity and other issues.


Anyway, what is the problem with accepting modem socket in ofono via
DBus and starts talking with it like with any other modem which ofono
supports? Currently ofono already takes modem socket from bluez via DBus
API, so in same way hsphfpd can pass modem socket to ofono. Basically
ofono then can reuse same code which already uses for sockets from
bluez, just it do not have to parse and interpret audio related AT
commands (as these are handled by hsphpfd itself).

What are exact issues? I do not see complexity at ofono part (as has
already everything implemented) and currently I do not see those "other"
issues.


The issues are many.  And really the question is not whether it 'could 
be' done, but whether it 'should be' done.  Let me describe a couple 
examples:


- In the case of HF role (1), oFono already provides all the necessary 
APIs.  So put yourself in oFono's maintainer shoes.  What would we gain 
by supporting almost the same but different mechanism?  We would have to 
introduce a new hfp_hf plugin, one that is almost identical, but 
different to hfp_hf_bluez5 plugin.  These two plugins would have 
coexistence issues, which would add more complexity.  Then there's the 
impact on PulseAudio and other users.  How do they know when to use the 
HandsfreeAudio API vs some external API?  Supporting this new way buys 
us a lot of extra code / complexity with no value added.


- The other example is more practical. HFP Service Level Connection 
(SLC) establishment is actually quite tricky.  There are certain 
limitations on what can and cannot be done prior to SLC establishment, 
including how audio handling is done.  Unfortunately, codec negotiation, 
indicator negotiation, and feature negotiation are part of the SLC. 
oFono already solves all of this and handles all of it nicely.  We have 
passed all relevant certification testing.  It is very unclear how you 
plan to handle this (or whether you realistically even can) in your 
architecture when the responsibilities are split between the various 
daemons.  So again, oFono has nothing to gain here...




You suggested to use phone simulator together with ofono and then
starts extending / patching phone simulator to supports all needed parts
which are in my hsphfpd design (battery status, button press, codec
selection)?



Not quite.  I suggested you expand oFono's emulator implementation (for 
AG role) and its DBus APIs (for HF role) to support the new vendor 
specific features that you want.


Forget about the phone simulator, it is really irrelevant for what 
you're trying to accomplish.



Also how to handle the main problem of phone simulator that it is too
much complicated to setup it on desktop? Looking at the steps...

https://www.freedesktop.org/wiki/Software/PulseAudio/Documentation/User/Bluetooth/#index5h2

... that desktop user needs to run nontrivial commands in command like,
plus creating configuration file only just to connect bluetooth headset
is ridiculous and non-acceptable for desktop application.

If this 

Re: [pulseaudio-discuss] Proposal for a new API and usage of Bluetooth HSP and HFP profiles on Linux

[Denis Kenzior]

Hi Pali,


But would you accept patches which exports DBus API e.g. for displaying text
on HFP headset with embedded display? Or patches which implements 3
different way for reporting battery level status of HFP headset? And API
for sending "computer battery level" to HFP headset? Or implementing
setup of SCO sockets (via RFCOMM layer) for custom codecs? Or exporting
uinput linux device for button press events? Because all these


So which roles are we talking about here?  Your Design document shows
hsphfpd registering for both HFP AG and HFP HF roles, but maybe this was not
the intent?


My proposed hsphfpd is going to support both roles. Which means to
implement whole HFP profile. So for connecting bluetooth headsets (when
AG role is needed on desktop) and also for connecting mobile phones
(when HF role is needed on desktop).

And my primary motivation is for bluetooth headsets as this is what are
asking desktop and laptop users again and again that is missing on Linux
systems.

So higher priority has AG role and slightly lower priority has HF role.



So to summarize.  You have broadly 3 main use cases for HFP:

1. HF connecting to AG role.  Essentially a carkit role.  oFono does 
this pretty well already and has the APIs defined that cover up to HFP 
1.7.  Any vendor extensions can be easily added.  And some carkit 
manufacturers already use it.


2. AG role when you have a 'real modem' behind it.  oFono already 
provides everything needed for this scenario.


3. AG role when you don't have a real modem or you have some sort of 
VoIP use case.  oFono doesn't cover this case as you stated.


So I can see value in something that implements case #3.  But having 
said that, oFono will not be receiving AT commands from external daemons:
	- For case 1, it'd just be a rehash of what oFono does well already.  I 
reinvented a few wheels in my time, but doesn't mean I think this one 
should be.

- The reasoning for case 2/3 I already covered upthread.


If you're talking about extending oFono APIs when it is acting as the HF
connecting to the AG, then codec setup APIs, etc are definitely something
that would be welcome.

If you're talking about AG role, then that is different... In general, if
the oFono is in an AG role, then there should be nothing to configure and
there are no APIs for this role.


Codecs needs to be configured also in AG role. Before accepting SCO
connection you need to configure SCO socket for correct codec. Also for
vendor codecs it needs to be properly negotiated via AT commands.



Sure, but that doesn't mean they need an actual D-Bus API to be 
configured with.  You can simply extend oFono emulator to support 
whatever codecs you want and whatever custom AT command handling that 
you need.  If the HF requests the codec, then you use it.  There's no 
D-Bus API required here.  Again, take a look at how this is done in 
oFono today.



Such a design will get a NAK from me on the oFono side.  But don't let that
stop you.  You can simply invoke oFono APIs directly from your daemon.  No
need for any changes in oFono itself.  As mentioned above, I wouldn't
recommend it, but... :)


So if you are disagreeing with this design, can you propose alternative?
Which would support needs for desktop users? Support for HSP profile (in
AG role), support for HFP profile (in AG role), ability to parse and
interpret needed AT commands. And later also HS and HF role of these
profiles.



There have been one or two implementations of AG role fully external to 
oFono.  These implementations simply used the existing oFono APIs to 
drive the modem.  You could do that.  But as I said, we rejected such a 
design a long time ago due to complexity and other issues.


Or you can ignore the call control aspects entirely...

But in the end, it is your architecture.  All I can do is point out 
(early in the process) what is and what is not acceptable to oFono upstream.




Okay, I see now.  Yes, the above is correct.  My comments about not needing
a modem device hold true only if oFono is in HFP HF role connecting to an
AG.


Ok. So I guess now you understood main problem. I thought it was
obvious, but seems that bluetooth HFP is too complicated, so talking
about it always needs more detailed explanation. Sorry for that if it
was not clear from my side since beginning what are requirements for my
setup.


Well it was a bit of reading comprehension fail on my part as well.  The 
two roles are really quite different, so precise language helps.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [pulseaudio-discuss] Proposal for a new API and usage of Bluetooth HSP and HFP profiles on Linux

[Denis Kenzior]

Hi Pali,


Yes, I see. Also there are devices without HFP support, only with HSP.
pulseaudio support also these devices and pulseaudio is not going to
drop support for them. Last time when I looked at ofono, it has no HSP
support. Is ofono going to add support for HSP?



No.





But would you accept patches which exports DBus API e.g. for displaying text
on HFP headset with embedded display? Or patches which implements 3
different way for reporting battery level status of HFP headset? And API
for sending "computer battery level" to HFP headset? Or implementing
setup of SCO sockets (via RFCOMM layer) for custom codecs? Or exporting
uinput linux device for button press events? Because all these


So which roles are we talking about here?  Your Design document shows 
hsphfpd registering for both HFP AG and HFP HF roles, but maybe this was 
not the intent?


If you're talking about extending oFono APIs when it is acting as the HF 
connecting to the AG, then codec setup APIs, etc are definitely 
something that would be welcome.


If you're talking about AG role, then that is different... In general, 
if the oFono is in an AG role, then there should be nothing to configure 
and there are no APIs for this role.  Things like reporting AG battery 
level to HFP headset are done directly using plugins.  See 
ofono_emulator_set_indicator and how this is done by upower.c for 
example.  I happily take patches for any additional features (codecs, 
etc) you want to support here.


But... oFono upstream has no interest in accepting forwarded AT commands 
from some external daemon if we're in AG role.  We rejected such a 
design years ago and nothing has changed.


AG role is already quite tricky to implement without additional 
complexity introduced by having multiple applications and IPC.  "Its 
your funeral" as the saying goes if you want to go that route.





I disagree here. Primary purpose of HFP for desktop users is ability to
use bluetooth's microphone. And not telephony stack and its complicated
features like call hold and others, which are in HFP used and
implemented probably only in car kits.


So your primary goal here is to have the desktop play the role of the 
AG, purely so you can forward the audio from a headset out to whatever 
it is that you want.  And you want oFono (or some other daemon) to 
(optionally) handle the call related AT commands in the HFP AG role.


Such a design will get a NAK from me on the oFono side.  But don't let 
that stop you.  You can simply invoke oFono APIs directly from your 
daemon.  No need for any changes in oFono itself.  As mentioned above, I 
wouldn't recommend it, but... :)





Also for using ofono with HFP profile is not possible on desktop
computer which do not have any modem as it is hooked to some active
modem.


This statement is not true at all.  You can use oFono without any 'real'
modems attached.  It can happily manage only HFP devices (or modems as we
call them.)


Ok, can you please provide exact steps how to do it, including
activating of bidirectional audio stream?


So again, which role?  If we're the HF connecting to the AG, then things 
just work without a modem.  If we're the AG, then yes you need a modem 
to be driven by the AG connection.




You must be thinking of the oFono HFP AG implementation...


Yes! For connecting bluetooth headset you need HFP AG implementation.

And I guess this is the reason why simulator is needed. HFP headset acts
as a "client" for modem. Therefore on desktop / laptop you need to
implement "modem server" which will be used by HFP headset "client".

And phone simulator is doing exactly this "modem server", it is
simulator of modem.



Okay, I see now.  Yes, the above is correct.  My comments about not 
needing a modem device hold true only if oFono is in HFP HF role 
connecting to an AG.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [pulseaudio-discuss] Proposal for a new API and usage of Bluetooth HSP and HFP profiles on Linux

[Denis Kenzior]

Hi Pali,


I'm not sure what logic around HSP you really care about.  It is just a
single button press in the end.


CSR features (battery status level, ...) and CSR codec selection (e.g.
AuriStream). Also some apple extensions are used in HSP profile.


Since HFP can do everything that HSP can and more, I view HSP as 
deprecated and not to be used.  If these are also available in HFP, then 
I'd just use HFP instead.  But HSP was never my focus, so if you feel 
there's a need for better HSP support, then fair enough.





For HFP, oFono can already support all sorts of extensions.  See for example
how we handled Siri for HFP support in oFono here:
https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/doc/siri-api.txt.


About Siri: In hsphfpd API it is delegated to Telephony Agent. So
hsphfpd is not going to (re)implement it.


I saw. But it does not support usage of vendor codecs, like CSR
AuriStream and it does not support CSR extensions, like displaying text
on embedded display.


But that's my point, you can easily accomplish this by creating another 
oFono API / atom for HFP CSR extensions and expose this information / 
functionality.  Similar to how Siri was done.  I see no need for a 
completely new external daemon.





Many of the extensions you talked about are also relevant for real modems as
well (like battery reporting, call volume, etc).  Some of these APIs are
already defined in fact.

Given the above, oFono upstream has no interest in adding or maintaining
support this new framework.


Maybe better question: Do you mean that you do not want to maintain
hsphfpd, or that you completely do not want to see that ofono would be
"Telepony Agent" for hsphfpd?


The latter.




Denis, if you are not interested in my proposed hsphfpd daemon, how you
want to solve problem with other extensions and other vendor codecs?



See above..


Also in my proposed solution it is possible to use HFP profile without
Telephony Agent (ofono). Do you think it is really a good idea to have
strong dependency on ofono just for bluetooth HFP headset?



Why not?  The main purpose of HFP is telephony; whether it is classic 
phone calls or skype/facetime.  oFono seems a natural fit.



Also for using ofono with HFP profile is not possible on desktop
computer which do not have any modem as it is hooked to some active
modem.


This statement is not true at all.  You can use oFono without any 'real' 
modems attached.  It can happily manage only HFP devices (or modems as 
we call them.)




There is a way to use ofono sim simulator which provide fake modem, but
its setup is hard on desktop and it not automated.



You must be thinking of the oFono HFP AG implementation...


So connecting bluetooth headset in HFP profile with ofono is something
not so easy and not an obvious way.



Again, not true.  As I said above, you can use oFono for this use case 
just fine.  Certainly the main driver for the development of oFono was 
to drive real modem hardware, but it isn't limited to that.  So if you 
want to use it only for HFP, you can.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [pulseaudio-discuss] Proposal for a new API and usage of Bluetooth HSP and HFP profiles on Linux

[Denis Kenzior]

Hi Pali,

On 12/16/19 3:15 AM, Pali Rohár wrote:

Hi!

On Monday 16 December 2019 00:11:04 Luiz Augusto von Dentz wrote:

Hi Pali,

On Thu, Dec 5, 2019 at 11:32 AM Pali Rohár  wrote:


On Monday 02 December 2019 19:45:12 Pali Rohár wrote:

On Monday 02 December 2019 19:01:11 Tanu Kaskinen wrote:

I think hsphfpd should be part of bluetoothd, but if that's not
possible, then that's not possible.


I do not know if bluez developers are interested in having this code as
part of bluez project, specially when in bluez4 HFP profile was there
and in bluez5 was HFP code completely removed.


Hello, could someone from bluez developers comment this Tanu's point?


I would have to say no, we are definitely not interested in yet
another daemon for AT parsing, we actually have too many of these
around, either in a form of Modem Manager, oFono, etc.


Proposed hsphfpd daemon is not (only) for parsing AT commands, but for
implementing logic around HSP and HFP profiles and export either native
interfaces (linux uinput) or DBus interfaces for features provided by
HSP and HFP specifications and also for current and future vendor
extensions. And part of this HSP/HFP implementation is of course needed
parsing and interpreting some of AT commands. Look into my design and
API proposal. Current daemons which provides AT parsing (like ofono or
modem manager) are not suitable for for whole HSP and HFP profiles with
all those extensions (like all possible ways for reporting battery
level), so for HFP is needed some of custom AT parser.


I'm not sure what logic around HSP you really care about.  It is just a 
single button press in the end.


For HFP, oFono can already support all sorts of extensions.  See for 
example how we handled Siri for HFP support in oFono here:
https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/doc/siri-api.txt. 
 Many of the extensions you talked about are also relevant for real 
modems as well (like battery reporting, call volume, etc).  Some of 
these APIs are already defined in fact.


Given the above, oFono upstream has no interest in adding or maintaining 
support this new framework.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 3/5] gemalto: gprs: support automatic context activation

[Denis Kenzior]

Hi Sergey,


Are you sure you don't want to wait until swwan_cb to return success to the
core?  AT^SWWAN can still fail...?


AT^SWWAN command is yet another way to activate a PDP context. AT commands
spec for this modem is a bit vague about SWWAN details, but according to
other materials from Gemalto as well as my experiments, this command
activates internal DHCP server, so DHCP client can be started for modem
USB ethernet interfaces. Based on my observations, SWWAN command does
not return until DHCP flow is completed.


Ugh.  I'd 'window.throw(modem)'...



So the idea is to send SWWAN command to modem and make it possible to
start DHCP flow right away. I assume that I need both things: mark
interface for DHCP and signal success to the core. Callback swwan_cb is


So the problem with this is that you've now blocked the app/modem port until 
that DHCP negotiation happens.  Maybe it does, maybe it doesn't.  It is less 
than ideal to depend on some external component; there are frequently situations 
where people would be running without ConnMan for example.



supposed to handle the case when SWWAN command fails: mark context as
deactivated and let oFono proceed with further connection attempts.



Another thing to consider is to just run dhcp yourself.  ell has had a DHCPv4 
client for a while now.  So you could just run l_dhcp_client to obtain the 
address and signal it to the core, leaving the app port in a known state...


Or better yet, don't use SWWAN if you can help it...




Sure, I can add a comment. What whould be better: to add a comment in
driver or to write a more detailed commit message ?


Given how unusual this behavior is, I'd add a comment directly in the code.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 5/5] gemalto: gprs: support authentication settings

[Denis Kenzior]

Hi Sergey,

On 12/21/20 2:01 PM, Sergey Matyukevich wrote:

From: Sergey Matyukevich 

Add support for gprs contexts with username, password, as well as
specific authentication type.
---
  drivers/gemaltomodem/gprs-context.c | 54 -
  1 file changed, 53 insertions(+), 1 deletion(-)



Looks good to me.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 3/5] gemalto: gprs: support automatic context activation

[Denis Kenzior]

Hi Sergey,

On 12/21/20 2:01 PM, Sergey Matyukevich wrote:

From: Sergey Matyukevich 

Implement read_settings function to get configuration for automatic
contexts. Fix the issue uncovered by added support for automatic
context activation: do not use AT+CGACT for the contexts handled
by AT^SWWAN. As per modem specs, CGACT context can not be reused
for SWWAN. Though that worked for some reason when automatic
context was reactivated without proper deactivation.
---
  drivers/gemaltomodem/gprs-context.c | 110 +++-
  1 file changed, 59 insertions(+), 51 deletions(-)

diff --git a/drivers/gemaltomodem/gprs-context.c 
b/drivers/gemaltomodem/gprs-context.c
index 322a5f98..680f01ab 100644
--- a/drivers/gemaltomodem/gprs-context.c
+++ b/drivers/gemaltomodem/gprs-context.c
@@ -50,70 +50,61 @@ struct gprs_context_data {
void *cb_data;
  };
  
-static void cgact_enable_cb(gboolean ok, GAtResult *result,

-   gpointer user_data)
+static void set_gprs_context_interface(struct ofono_gprs_context *gc)
  {
-   struct ofono_gprs_context *gc = user_data;
-   struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
struct ofono_modem *modem;
const char *interface;
-   char buf[64];
+
+   modem = ofono_gprs_context_get_modem(gc);
+   interface = ofono_modem_get_string(modem, "NetworkInterface");
+   ofono_gprs_context_set_interface(gc, interface);
+}
+
+static void swwan_cb(gboolean ok, GAtResult *result, gpointer user_data)
+{
+   struct ofono_gprs_context *gc = user_data;
+   struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+   struct ofono_error error;
  
  	DBG("ok %d", ok);
  
  	if (!ok) {

-   struct ofono_error error;
-
+   ofono_error("Unable to activate context");
+   ofono_gprs_context_deactivated(gc, gcd->active_context);
gcd->active_context = 0;


This seems a bit strange.  Are you signaling success to the core too early


-
decode_at_error(, g_at_result_final_response(result));
gcd->cb(, gcd->cb_data);
-
return;
}
-
-   snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
-   g_at_chat_send(gcd->chat, buf, none_prefix, NULL, NULL, NULL);
-
-   modem = ofono_gprs_context_get_modem(gc);
-   interface = ofono_modem_get_string(modem, "NetworkInterface");
-   ofono_gprs_context_set_interface(gc, interface);
-
-   /* Use DHCP */
-   ofono_gprs_context_set_ipv4_address(gc, NULL, 0);
-
-   CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);
  }
  
  static void cgdcont_enable_cb(gboolean ok, GAtResult *result,

-   gpointer user_data)
+   gpointer user_data)


nit: This is superfluous and also see doc/coding-style.txt item M4


  {
struct ofono_gprs_context *gc = user_data;
struct gprs_context_data *gcd = ofono_gprs_context_get_data(gc);
+   struct ofono_error error;
char buf[64];
  
  	DBG("ok %d", ok);
  
  	if (!ok) {

-   struct ofono_error error;
-
gcd->active_context = 0;
-
decode_at_error(, g_at_result_final_response(result));
gcd->cb(, gcd->cb_data);
-
return;
}
  
-	snprintf(buf, sizeof(buf), "AT+CGACT=1,%u", gcd->active_context);

-
-   if (g_at_chat_send(gcd->chat, buf, none_prefix,
-   cgact_enable_cb, gc, NULL) == 0)
-   goto error;
+   snprintf(buf, sizeof(buf), "AT^SWWAN=1,%u", gcd->active_context);
+   if (g_at_chat_send(gcd->chat, buf, none_prefix, swwan_cb, gc, NULL)) {


Note, this returns > 0 when the command has been queued, not that it has been 
sent or replied to yet...



+   set_gprs_context_interface(gc);
+   /* Use DHCP */
+   ofono_gprs_context_set_ipv4_address(gc, NULL, 0);


If these modems can only do DHCP, then might be cleaner to move the 'Use DHCP' 
bits into set_gprs_context_interface.  And maybe rename it to make things 
clearer, if needed.


  
-	return;

+   CALLBACK_WITH_SUCCESS(gcd->cb, gcd->cb_data);


Are you sure you don't want to wait until swwan_cb to return success to the 
core?  AT^SWWAN can still fail...?



+   return;
+   }
  
-error:

CALLBACK_WITH_FAILURE(gcd->cb, gcd->cb_data);
  }
  





@@ -185,17 +170,39 @@ static void gemalto_gprs_deactivate_primary(struct 
ofono_gprs_context *gc,
gcd->cb = cb;
gcd->cb_data = data;
  
-	snprintf(buf, sizeof(buf), "AT+CGACT=0,%u", cid);

+   snprintf(buf, sizeof(buf), "AT^SWWAN=0,%u", gcd->active_context);
  
  	if (g_at_chat_send(gcd->chat, buf, none_prefix,

-   cgact_disable_cb, gc, NULL) == 0)
-   goto error;
-
-   return;
+   deactivate_cb, gc, NULL))
+   return;
  

Re: [PATCH 1/5] plugin: gemalto: fix source of gprs notifications

[Denis Kenzior]

Hi Sergey,

On 12/21/20 2:01 PM, Sergey Matyukevich wrote:

Modem USB interface does not receive certain gprs context notifications.
Fix gprs chat: use Application USB interface to receive all the modem
notifications.
---
  plugins/gemalto.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)



Patch 1, 2 and 4 applied,  thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] huawei, util: Fix a couple of implicit enum conversions

[Denis Kenzior]

Hi Richard,

On 12/20/20 5:18 AM, richard.rojf...@gmail.com wrote:

From: Richard Röjfors 

GCC 10 warns about a couple of implicit conversions;

huawei: Member from the incorrect enum was returned,
both had the value 0, so the code would still work.

drivers/huaweimodem/radio-settings.c: In function ‘band_gsm_from_huawei’:
drivers/huaweimodem/radio-settings.c:107:10: error: implicit conversion from 
‘enum ofono_radio_band_umts’ to ‘enum ofono_radio_band_gsm’ 
[-Werror=enum-conversion]
   107 |   return OFONO_RADIO_BAND_UMTS_ANY;

util: smsutil and util has an enum each for representing
the same thing; The SMS alphabet. They share the same
values, so an explicit type cast makes GCC happy.

src/smsutil.c: In function ‘sms_text_prepare_with_alphabet’:
src/smsutil.c:3594:8: error: implicit conversion from ‘enum sms_alphabet’ to 
‘enum gsm_dialect’ [-Werror=enum-conversion]
  3594 |alphabet, _locking,
---
  drivers/huaweimodem/radio-settings.c | 2 +-
  src/smsutil.c| 5 -
  2 files changed, 5 insertions(+), 2 deletions(-)



I split this into two commits and applied.  Thanks!

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v3] quectel: Power on/off with a gpio pulse

[Denis Kenzior]

Hi Lars,

On 10/6/20 5:10 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

Current implementation uses a gpio level of 1 for powering on quectel
modems using a gpio and a level of 0 for powering off.
Normally quectel modems are powered on or off by a gpio pulse on their
PWR_KEY pin. They turn on by the first pulse and turn then off by the
next pulse. The pulse length varies between different modems.
For power on the longest I could in the quectel hardware is "more than
2 seconds" from Quectel M95 Hardware Design Manual.
For Quectel EC21 this is ">= 100 ms".
For Quectel MC60 this is "recommended to be 100 ms".
For Quectel UC15 this is "at least 0.1 s".
For power off the four modems in question vary between a minimum pulse
length of 600-700ms.
This implements a 2100ms pulse for power on and 750ms for power off.

If you have some special circuitry that powers your modem by gpio level
and you need the old behaviour, you can switch to gpio level powering
by setting environment variable OFONO_QUECTEL_GPIO_LEVEL. The gpio goes
to high level for the modem to power on and to low level if it should
power off.

---
Changes in v3:
- switched to l_timeout_create_ms instead of g_timeout for the gpio
   pulse
- try to keep track of the gpio_timeout object
- combined patch with the level patch
---
  plugins/quectel.c | 45 +++--
  plugins/udevng.c  |  5 +
  2 files changed, 48 insertions(+), 2 deletions(-)



Applied with a minor tweak:


+static void gpio_power_off_cb(struct l_timeout *timeout, void *user_data)
+{
+   struct ofono_modem *modem = (struct ofono_modem *)user_data;


This cast was not needed


+   struct quectel_data *data = ofono_modem_get_data(modem);
+   const uint32_t gpio_value = 0;
+
+   l_timeout_remove(timeout);
+   data->gpio_timeout = NULL;
+   l_gpio_writer_set(data->gpio, 1, _value);
+   ofono_modem_set_powered(modem, FALSE);
+}
+


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] quectel: Possibility to switch power by gpio level

[Denis Kenzior]

Hi Lars,

On 10/2/20 7:49 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

This does apply on top of my previous patch:
[PATCH v2] quectel: Power on/off with a gpio pulse


I think you might as well just combine the two...

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] quectel: Power on/off with a gpio pulse

[Denis Kenzior]

Hi Lars,

On 10/1/20 6:42 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

Current implementation uses a gpio level of 1 for powering on quectel
modems using a gpio and a level of 0 for powering off.
This is wrong. Quectel modems use pulses for either power on and power
off. They turn on by the first pulse and turn then off by the next
pulse. The pulse length varies between different modems.
For power on the longest I could in the quectel hardware is "more than
2 seconds" from Quectel M95 Hardware Design Manual.
For Quectel EC21 this is ">= 100 ms".
For Quectel MC60 this is "recommended to be 100 ms".
For Quectel UC15 this is "at least 0.1 s".
For power off the four modems in question vary between a minimum pulse
length of 600-700ms.
This implements a 2100ms pulse for power on and 750ms for power off.
---
  plugins/quectel.c | 33 ++---
  1 file changed, 30 insertions(+), 3 deletions(-)

diff --git a/plugins/quectel.c b/plugins/quectel.c
index 6456775d..61ac906e 100644
--- a/plugins/quectel.c
+++ b/plugins/quectel.c
@@ -234,10 +234,21 @@ static void close_ngsm(struct ofono_modem *modem)
ofono_warn("Failed to restore line discipline");
  }
  
+static gboolean gpio_power_off_cb(gpointer user_data)

+{
+   struct ofono_modem *modem = (struct ofono_modem *)user_data;
+   struct quectel_data *data = ofono_modem_get_data(modem);
+   const uint32_t gpio_value = 0;
+
+   l_gpio_writer_set(data->gpio, 1, _value);
+   ofono_modem_set_powered(modem, FALSE);
+   return false;
+}
+


Ok, this makes sense now...


  static void close_serial(struct ofono_modem *modem)
  {
struct quectel_data *data = ofono_modem_get_data(modem);
-   uint32_t gpio_value = 0;
+   uint32_t gpio_value = 1;
  
  	DBG("%p", modem);
  
@@ -258,8 +269,12 @@ static void close_serial(struct ofono_modem *modem)

else
close_ngsm(modem);
  
-	l_gpio_writer_set(data->gpio, 1, _value);

-   ofono_modem_set_powered(modem, FALSE);
+   if (data->gpio) {
+   l_gpio_writer_set(data->gpio, 1, _value);
+   g_timeout_add(750, gpio_power_off_cb, modem);


Have you considered what happens if the gpio_power_on_cb timeout is still 
running here?  For example, if the modem is turned on / off quickly?


Maybe the old timeout should be canceled just in case?


+   } else
+   ofono_modem_set_powered(modem, FALSE);
+
  }
  
  static void dbus_hw_reply_properties(struct dbus_hw *hw)

@@ -1096,6 +,15 @@ static void init_timeout_cb(struct l_timeout *timeout, 
void *user_data)
l_timeout_modify_ms(timeout, 500);
  }
  
+static gboolean gpio_power_on_cb(gpointer user_data)

+{
+   struct quectel_data *data = user_data;
+   const uint32_t gpio_value = 0;
+
+   l_gpio_writer_set(data->gpio, 1, _value);
+   return false;
+}


It seems that this timeout is completely independent of 
ofono_modem_set_powered(TRUE), so the core won't prevent the modem from being 
powered off while this is running...



+
  static int open_serial(struct ofono_modem *modem)
  {
struct quectel_data *data = ofono_modem_get_data(modem);
@@ -1139,6 +1163,9 @@ static int open_serial(struct ofono_modem *modem)
return -EIO;
}
  
+	if (data->gpio)

+   g_timeout_add(2100, gpio_power_on_cb, data);
+


Generally it is a good idea to keep track of any timeout objects you're adding. 
This is especially true on hot-plug/unplug capable hardware since the modem 
object and its data might go away, but the timer would still be running, causing 
a SIGSEGV later.


Granted this is a serial device, so this won't likely happen in this case...


/*
 * there are three different power-up scenarios:
 *



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 2/2] quectel: Power on/off with a gpio pulse

[Denis Kenzior]

Hi Lars,


Can this be done using g_timeout instead?


This was indeed not the first solution I tried. But I used the l_timeout_
thing that is used all over in quectel.c. I could not get this to work
in a way I was satisfied with. When I send a SIGTERM to the ofono
process close_serial is called but the timeout_callback never fires


It seems this is because you call ofono_modem_set_powered(..., FALSE) in 
close_serial().  What you should be doing is delaying this until your gpio 
operation is complete.



and thus the modem does not power off. This is why I then tried usleep
and this works reliably.
Just to be sure I tried with g_timeout and this is the same like
l_timeout. The callback is not called.


See above, l_timeout / g_timeout won't matter here...  The driver needs to tell 
the core when it is done powering up / powering down.  This is accomplished by 
returning -EINPROGRESS from .enable/.disable and calling 
ofono_modem_set_powered() once the operation completes.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/1] remove obsolete m4 macro

[Denis Kenzior]

Hi Nikhil,

On 8/25/20 3:12 PM, Nikhil Jha wrote:

As per https://www.gnu.org/software/libtool/manual/html_node/LT_005fINIT.html
LT_INIT should be used instead.
---
  configure.ac | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH] common: APN with a dot in second position are not invalid

[Denis Kenzior]

Hi Christophe,

On 9/28/20 2:38 AM, Christophe Ronco wrote:

APN with a dot in second position (example: "t.est") are wrongly
considered invalid.
---
  src/common.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 3/3] droid 4: Add probing.

[Denis Kenzior]

Hi Pavel,

On 9/15/20 8:23 AM, Pavel Machek wrote:

Probe Droid 4 modem. This should result in basic support working.
---
  Makefile.am  |   3 +
  plugins/droid.c  | 206 +++
  plugins/udevng.c |  31 +
  3 files changed, 240 insertions(+)
  create mode 100644 plugins/droid.c



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 2/3] droid 4: special handling for SMSes

[Denis Kenzior]

Hi Pavel,

On 9/15/20 8:23 AM, Pavel Machek wrote:

Droid 4  modem is "special" (aka broken) so and getting incoming SMSes
to work is quite tricky. This should get it right.
---
  drivers/atmodem/sms.c | 15 ---
  1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index d502da72..f08f2fb1 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -463,7 +463,8 @@ static void at_cmt_notify(GAtResult *result, gpointer 
user_data)
decode_hex_own_buf(hexpdu, -1, _len, 0, pdu);
ofono_sms_deliver_notify(sms, pdu, pdu_len, tpdu_len);
  
-	if (data->vendor != OFONO_VENDOR_SIMCOM)

+   if ((data->vendor != OFONO_VENDOR_SIMCOM) &&
+   (data->vendor != OFONO_VENDOR_DROID))


Fixed up style here per item M4


at_ack_delivery(sms);
return;
  





@@ -858,9 +860,14 @@ static gboolean build_cnmi_string(char *buf, int 
*cnmi_opts,
if (!append_cnmi_element(buf, , cnmi_opts[0], mode, FALSE))
return FALSE;
  
+	mode = "21";

+   if (!data->cnma_enabled)
+   mode = "1";
+   if (data->vendor == OFONO_VENDOR_DROID)
+   mode = "2";
+


And here per item M1.

Also, I had to resolve a minor conflict manually.  Please rebase against git 
HEAD when sending out patches.


Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/3] droid 4: add special handling required for voice calls and SIM

[Denis Kenzior]

Hi Pavel,

On 9/15/20 8:23 AM, Pavel Machek wrote:

Droid 4 modem is "special" (aka broken) so it seems to need a bit of
error handling.
---
  drivers/atmodem/sim.c   | 1 +
  drivers/atmodem/vendor.h| 1 +
  drivers/atmodem/voicecall.c | 4 
  3 files changed, 6 insertions(+)






@@ -160,6 +160,10 @@ static void clcc_poll_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
poll_again = TRUE;
goto poll_again;
}


applied with an extra newline here to satisfy doc/coding-style.txt item M1.


+   if (vd->vendor == OFONO_VENDOR_DROID) {
+   poll_again = TRUE;
+   goto poll_again;
+   }
  
  		ofono_error("We are polling CLCC and received an error");

ofono_error("All bets are off for call management");



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v3] Support weird modem in Motorola Droid 4

[Denis Kenzior]

Hi Pavel,

On 9/10/20 8:05 AM, Pavel Machek wrote:

This is basic support for modem in Droid 4, accessed over
ttyUSB4. That interface is unfortunately quite broken, so we need to
force very specific SMS mode.



Can you separate this into at least a couple of patches per our patch submission 
guidelines? See HACKING, 'Submitting Patches' section.


1 - atmodem / vendor quirks
2 - plugins/droid.c + build changes


---

Resent.

Best regards,
Pavel

diff --git a/Makefile.am b/Makefile.am
index fbb0eff4..9b3fbb8d 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -603,6 +603,9 @@ builtin_sources += plugins/ublox.c
  builtin_modules += xmm7xxx
  builtin_sources += plugins/xmm7xxx.c
  
+builtin_modules += droid

+builtin_sources += plugins/droid.c
+
  if BLUETOOTH
  if BLUEZ4
  builtin_modules += sap
diff --git a/drivers/atmodem/sim.c b/drivers/atmodem/sim.c
index e750a139..f46cd3a2 100644
--- a/drivers/atmodem/sim.c
+++ b/drivers/atmodem/sim.c
@@ -199,6 +199,7 @@ static void at_sim_read_info(struct ofono_sim *sim, int 
fileid,
case OFONO_VENDOR_SPEEDUP:
case OFONO_VENDOR_QUALCOMM_MSM:
case OFONO_VENDOR_SIMCOM:
+   case OFONO_VENDOR_DROID:
/* Maximum possible length */
len += sprintf(buf + len, ",0,0,255");
break;
diff --git a/drivers/atmodem/sms.c b/drivers/atmodem/sms.c
index d502da72..a9306916 100644
--- a/drivers/atmodem/sms.c
+++ b/drivers/atmodem/sms.c
@@ -349,8 +349,15 @@ static inline void at_ack_delivery(struct ofono_sms *sms)
break;
}
} else {
-   /* Should be a safe fallback */
-   snprintf(buf, sizeof(buf), "AT+CNMA=0");
+   switch (data->vendor) {
+   case OFONO_VENDOR_DROID:
+   snprintf(buf, sizeof(buf), "AT");
+   break;
+   default:
+   /* Should be a safe fallback */
+   snprintf(buf, sizeof(buf), "AT+CNMA=0");
+   break;
+   }


I'd rather you just not call at_ack_delivery at all in the case of DROID.  In 
fact, I don't think this function gets invoked at all if cnma_enabled is false...



}
  
  	g_at_chat_send(data->chat, buf, none_prefix, at_cnma_cb, NULL, NULL);

@@ -845,6 +852,7 @@ static gboolean build_cnmi_string(char *buf, int *cnmi_opts,
case OFONO_VENDOR_ZTE:
case OFONO_VENDOR_SIMCOM:
case OFONO_VENDOR_QUECTEL:
+   case OFONO_VENDOR_DROID:
/* MSM devices advertise support for mode 2, but return an
 * error if we attempt to actually use it. */
mode = "1";
@@ -858,9 +866,14 @@ static gboolean build_cnmi_string(char *buf, int 
*cnmi_opts,
if (!append_cnmi_element(buf, , cnmi_opts[0], mode, FALSE))
return FALSE;
  
+	mode = "21";

+   if (!data->cnma_enabled)
+   mode = "1";
+   if (data->vendor == OFONO_VENDOR_DROID)
+   mode = "2";
+


So you want to deliver via +CMT but not do CNMA acks.  Ok I guess...


/* Prefer to deliver SMS via +CMT if CNMA is supported */
-   if (!append_cnmi_element(buf, , cnmi_opts[1],
-   data->cnma_enabled ? "21" : "1", FALSE))
+   if (!append_cnmi_element(buf, , cnmi_opts[1], mode, FALSE))
return FALSE;
  
  	switch (data->vendor) {

@@ -1243,7 +1256,9 @@ static void at_csms_status_cb(gboolean ok, GAtResult 
*result,
goto out;
  
  		if (service == 1 || service == 128)

-   data->cnma_enabled = TRUE;
+   if (data->vendor != OFONO_VENDOR_DROID) {
+   data->cnma_enabled = TRUE;
+   }




...which should be false on DROID...?  Also, no need for {}.  See 
doc/coding-style.txt for details.


  
  		if (mt == 1 && mo == 1)

supported = TRUE;
@@ -1290,6 +1305,8 @@ static void at_csms_query_cb(gboolean ok, GAtResult 
*result,
goto out;
  
  	switch (data->vendor) {

+   case OFONO_VENDOR_DROID:
+   break;
case OFONO_VENDOR_QUECTEL_SERIAL:
g_at_result_iter_next_number(, _min);
g_at_result_iter_next_number(, _max);
diff --git a/drivers/atmodem/vendor.h b/drivers/atmodem/vendor.h
index d839d1e0..2bfd3eb8 100644
--- a/drivers/atmodem/vendor.h
+++ b/drivers/atmodem/vendor.h
@@ -27,6 +27,7 @@ enum ofono_vendor {
OFONO_VENDOR_MBM,
OFONO_VENDOR_GOBI,
OFONO_VENDOR_QUALCOMM_MSM,
+   OFONO_VENDOR_DROID,
OFONO_VENDOR_OPTION_HSO,
OFONO_VENDOR_ZTE,
OFONO_VENDOR_HUAWEI,
diff --git a/drivers/atmodem/voicecall.c b/drivers/atmodem/voicecall.c
index 7ab6567f..6c80bd4e 100644
--- a/drivers/atmodem/voicecall.c
+++ b/drivers/atmodem/voicecall.c
@@ -160,6 +160,10 @@ static void 

Re: Supporting modem in Droid 4

[Denis Kenzior]

Hi Pavel,

On 9/9/20 6:35 AM, Pavel Machek wrote:

Hi!

Two months I sent patch for Droid 4 support in ofono:

https://www.mail-archive.com/ofono@ofono.org/msg19590.html

I got no replies. Could I get some comments? Should I try to resend?


Please resend.  Looks like they got buried in a thread and I must have missed 
these.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v3] atmodem: Detect usage of AT+CGEREP

[Denis Kenzior]

Hi Lars,

On 9/1/20 4:21 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

Currently AT+CGEREP=2,1 is sent in case we don't know what the modem
needs. (default case) Not all modems understand this. So, we first query
what the modem supports with AT+CGEREP=? and then use this information
to be nice to the modem. This way modems, like the Quectel M95 that do
only understand AT+CGEREP=1 do also work nicely.
---
Changes since v2:
- (min <= 1 <= max)
- error handling



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] atmodem: Detect usage of AT+CGEREP

[Denis Kenzior]

Hi Lars,

On 8/31/20 8:24 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

Currently AT+CGEREP=2,1 is sent in case we don't know what the modem
needs. (default case) Not all modems understand this. So, we first query
what the modem supports with AT+CGEREP=? and then use this information
to be nice to the modem. This way modems, like the Quectel M95 that do
only understand AT+CGEREP=1 do also work nicely.
---
  drivers/atmodem/gprs.c | 59 --
  1 file changed, 57 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index 68470f11..443bdf77 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -45,6 +45,7 @@
  #define MAX_CONTEXTS 255
  
  static const char *cgreg_prefix[] = { "+CGREG:", NULL };

+static const char *cgerep_prefix[] = { "+CGEREP:", NULL };
  static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
  static const char *cgact_prefix[] = { "+CGACT:", NULL };
  static const char *none_prefix[] = { NULL };
@@ -648,6 +649,60 @@ static void gprs_initialized(gboolean ok, GAtResult 
*result, gpointer user_data)
ofono_gprs_register(gprs);
  }
  
+static void at_cgerep_test_cb(gboolean ok, GAtResult *result,

+   gpointer user_data)
+{
+   struct ofono_gprs *gprs = user_data;
+   struct gprs_data *gd = ofono_gprs_get_data(gprs);
+   GAtResultIter iter;
+   int min, max, arg1 = 0, arg2 = 0;
+   gboolean two_arguments = true;
+   char buf[20];
+
+   if (!ok)
+   return;
+
+   g_at_result_iter_init(, result);
+
+   g_at_result_iter_next(, "+CGEREP:");
+
+   if (!g_at_result_iter_open_list())
+   return;
+
+   while (g_at_result_iter_next_range(, , )) {
+   if ((min <= 1) && (max >= 1))
+   arg1 = 1;
+
+   if ((min <= 2) && (max >= 2))
+   arg1 = 2;
+   }
+
+   if (!g_at_result_iter_close_list())
+   return;
+
+   if (!g_at_result_iter_open_list()) {
+   two_arguments = false;
+   goto out;
+   }
+
+   while (g_at_result_iter_next_range(, , )) {
+   if (min <= 1 <= max)
+   arg2 = 1;


drivers/atmodem/gprs.c: In function ‘at_cgerep_test_cb’:
drivers/atmodem/gprs.c:689:11: error: comparisons like ‘X<=Y<=Z’ do not have 
their mathematical meaning [-Werror=parentheses]



+   }
+
+   if (!g_at_result_iter_close_list())
+   return;
+
+out:
+   if (two_arguments)
+   sprintf(buf, "AT+CGEREP=%u,%u", arg1, arg2);
+   else
+   sprintf(buf, "AT+CGEREP=%u", arg1);
+
+   g_at_chat_send(gd->chat, buf, none_prefix, gprs_initialized, gprs,
+   NULL);
+}
+
  static void at_cgreg_test_cb(gboolean ok, GAtResult *result,
gpointer user_data)
  {
@@ -702,8 +757,8 @@ retry:
gprs_initialized, gprs, NULL);
break;
default:
-   g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
-   gprs_initialized, gprs, NULL);
+   g_at_chat_send(gd->chat, "AT+CGEREP=?", cgerep_prefix,
+   at_cgerep_test_cb, gprs, NULL);
break;
}
  



Regards,
-Deenis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP

[Denis Kenzior]

Hi Lars,


So in theory this could be written like:
int max = -1;
iter_open_list();

while (iter_next_range(, , )) {
if (max2 > max)
max = max2;
}

iter_close_list();


I can do this and it does indeed work! :-)

This may be my personal thing, but I find this not very intuitive and
hard to read.

To make this more intuitive can we:
* move your proposed while loop to a new function
* name that function ...iter_next_range_or_list (or something like that)
* put that function into gatresult.c


I'm confused.  iter_next_range specifically handles ranges as defined by ITU 
v.250.  See section 5.7.3.1 "Range of Values".


Here's a small quote:
"If more than one value is supported, then the values may be listed 
individually, separated by comma characters (IA5 2/12), or, when a continuous 
range of values is supported, by the first value in the range, followed by a 
hyphen character (IA5 2/13), followed by the last value in the range. The 
specification of single values and ranges of values may be intermixed within a 
single information text. In all cases, the supported values shall be indicated 
in ascending order."


The only thing the user has to do is open up a list, since some modems get this 
wrong and don't enclose the entire set in '()'



* and in the new at_cgerep_test_cb just use this function ?


How would you return the set of supported settings?  I guess you could use 
something like l_uintset, but even knowing the valid theoretical range is tricky.




I can imagine this new function can be of use at other places as well.
What do you think ?


Given my confusion above, no idea?

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2] atmodem: Signal quality on quectel serial modems

[Denis Kenzior]

Hi Lars,

On 8/21/20 4:24 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

As the default way of getting the signal quality with +CIND is also
unstable on quectel serial modems (the same as on quectel EC21). In fact
the signal quality is only updated on cell changes. Those trigger a
manual AT+CSQ in ofono and get an update this way, but the URCs do not
work.
So we implement a quectelish way here as well. The quectelish way is
very similar to the way ifx modems work. We can reuse their csq_notify
function.
---
  drivers/atmodem/network-registration.c | 24 +---
  1 file changed, 21 insertions(+), 3 deletions(-)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP

[Denis Kenzior]

Hi Lars,


So why not just run iter_next_range in a loop?  it actually understands both
lists and ranges.  See cind_support_cb() for an example.


Ok, I can do this i a loop if you want.


+   /* if min1 == max1 we had no range but a list and that
+* means we are interested in the last number of that list*/
+   if (min1 == max1) {
+   while (!g_at_result_iter_close_list()) {
+   if (!g_at_result_iter_next_number(, ))
+   break;
+   }
+   }
+
+   if (!g_at_result_iter_skip_next()) {
+   two_arguments = false;
+   goto out;
+   }


Not sure what this is doing?  isn't +CGEREP just two lists ?  According to 
27.007:
"+CGEREP: (list of supported s),(list of supported s)"


Well, we have to deal with very different +CGEREP results. For example
on the quectel EC21 I get this:

"+CGEREP: (0-2),(0,1)"

and on the quectel M95 I get this:

"+CGEREP: (0,1)"

So what the code does is this:
It tries to parse a range with

g_at_result_iter_next_range(, , )

Now two things can happen: It either parsed the range "(0-2)" then we
have min1 != max1, or it tried to parse a list "(0,1)". This time
iter_next_range() breaks on the comma ',' and exits with
min1 == max1 == 0. Then we know, we did not find the maximum value yet
and we enter the loop:

▸··▸···while (!g_at_result_iter_close_list()) {
▸··▸···▸···if (!g_at_result_iter_next_number(, ))
▸··▸···▸···▸···break;
▸··▸···}

This does then loop to the end of the list until iter_close_list()
becomes true, which is at the closing paranthesis ')'. max1 then
contains the last item in that list (which we suspect to be the
maximum value).


So in theory this could be written like:
int max = -1;
iter_open_list();

while (iter_next_range(, , )) {
if (max2 > max)
max = max2;
}

iter_close_list();



Now we have min1 and max1 in both cases(list and range). Now if

g_at_result_iter_skip_next()

fails, we are at the end of the AT result and we had only one argument
to +CGEREP or we start trying to parse the second argument also. (See
below.)

At the end we construct our AT+CGEREP= string depending on if we had
two or only one argument.


or you could take the above code I suggested followed by something like:

if (!iter_open_list()) {
twoargs = false;
goto done;
}

while (iter_next_range(...)) {
...
}

iter_close_list();

...

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [RFC PATCH] atmodem: Signal quality on quectel serial modems

[Denis Kenzior]

Hi Lars,

On 8/19/20 7:43 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

As the default way of getting the signal quality with +CIND is also
unstable on quectel serial modems (the same as on quectel EC21). In fact
the signal quality is only updated on cell changes. Those trigger a
manual AT+CSQ in ofono and get an update this way, but the URCs do not
work.
So we implement a quectelish way here as well.

--- >8 ---

I send this patch as a RFC because the quectel_csqn_notify function
very much duplicates the ifx_xcsq_notify function despite the "+CSQN"
string. I did not see a good way to reuse the already existing
function because the callback interface only has one user defined
pointer and this is used by the struct ofono_netreg pointer already.
Does anyone have a better idea ?


You could take advantage of the fact that at_netreg_data carries the vendor id 
in nd->vendor.  So if you wanted to make ifx_xcsq_notify you could do something 
like:


struct ofono_netreg *netreg = user_data;
struct at_netreg_data *nd =
ofono_netreg_get_data(netreg);

switch (nd->vendor) {
case ...
prefix = "+XCSQ:";
break;
case ...
prefix = "+CSQN:";
break;
}



Or just put the meat of ifx_xcsq_notify into a separate function that takes the 
prefix as a parameter...


Then ifx_xcsq_notify might look something like:

netreg_generic_strength_report("+XCSQ:");



Thanks,
Lars
---
  drivers/atmodem/network-registration.c | 34 ++
  1 file changed, 34 insertions(+)



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 1/2] atmodem: Detect usage of AT+CGEREP

[Denis Kenzior]

Hi Lars,

On 8/19/20 7:13 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

Currently AT+CGEREP=2,1 is sent in case we don't know what the modem
needs. (default case) Not all modems understand this. So, we first query
what the modem supports with AT+CGEREP=? and then use this information
to be nice to the modem. This way modems, like the Quectel M95 that do
only understand AT+CGEREP=1 do also work nicely.


I think this is a nice improvement, couple of comments below:


---
  drivers/atmodem/gprs.c | 75 --
  1 file changed, 73 insertions(+), 2 deletions(-)

diff --git a/drivers/atmodem/gprs.c b/drivers/atmodem/gprs.c
index d829e2e6..3900b95b 100644
--- a/drivers/atmodem/gprs.c
+++ b/drivers/atmodem/gprs.c
@@ -45,6 +45,7 @@
  #define MAX_CONTEXTS 255
  
  static const char *cgreg_prefix[] = { "+CGREG:", NULL };

+static const char *cgerep_prefix[] = { "+CGEREP:", NULL };
  static const char *cgdcont_prefix[] = { "+CGDCONT:", NULL };
  static const char *cgact_prefix[] = { "+CGACT:", NULL };
  static const char *none_prefix[] = { NULL };
@@ -647,6 +648,76 @@ static void gprs_initialized(gboolean ok, GAtResult 
*result, gpointer user_data)
ofono_gprs_register(gprs);
  }
  
+static void at_cgerep_test_cb(gboolean ok, GAtResult *result,

+   gpointer user_data)
+{
+   struct ofono_gprs *gprs = user_data;
+   struct gprs_data *gd = ofono_gprs_get_data(gprs);
+   GAtResultIter iter;
+   int min1, max1 = 1, min2, max2 = 1;
+   gboolean two_arguments = true;
+   char buf[20];
+
+   if (!ok)
+   return;


Hmm, maybe a good idea here is to print a warning and call ofono_gprs_remove(). 
Otherwise this will just fail silently for someone.



+
+   g_at_result_iter_init(, result);
+
+   g_at_result_iter_next(, "+CGEREP:");
+
+   if (!g_at_result_iter_open_list())
+   return;


Ditto here


+
+   if (!g_at_result_iter_next_range(, , ))
+   return;
+


So why not just run iter_next_range in a loop?  it actually understands both 
lists and ranges.  See cind_support_cb() for an example.



+   /* if min1 == max1 we had no range but a list and that
+* means we are interested in the last number of that list*/
+   if (min1 == max1) {
+   while (!g_at_result_iter_close_list()) {
+   if (!g_at_result_iter_next_number(, ))
+   break;
+   }
+   }
+
+   if (!g_at_result_iter_skip_next()) {
+   two_arguments = false;
+   goto out;
+   }


Not sure what this is doing?  isn't +CGEREP just two lists ?  According to 
27.007:
"+CGEREP: (list of supported s),(list of supported s)"


+
+   if (!g_at_result_iter_open_list()) {
+   two_arguments = false;
+   goto out;
+   }
+
+   if (!g_at_result_iter_next_range(, , )) {
+   two_arguments = false;
+   goto out;
+   }
+
+   if (min2 == max2) {
+   while (!g_at_result_iter_close_list()) {
+   if (!g_at_result_iter_next_number(, )) {
+   break;
+   }
+   }
+   }
+
+out:
+   if (max1 > 2)
+   max1 = 2;
+
+   if (max2 > 1)
+   max2 = 1;
+
+   if (two_arguments)
+   sprintf(buf, "AT+CGEREP=%u,%u", max1, max2);
+   else
+   sprintf(buf, "AT+CGEREP=%u", max1);
+
+   g_at_chat_send(gd->chat, buf, none_prefix, gprs_initialized, gprs, 
NULL);
+}
+
  static void at_cgreg_test_cb(gboolean ok, GAtResult *result,
gpointer user_data)
  {
@@ -701,8 +772,8 @@ retry:
gprs_initialized, gprs, NULL);
break;
default:
-   g_at_chat_send(gd->chat, "AT+CGEREP=2,1", none_prefix,
-   gprs_initialized, gprs, NULL);
+   g_at_chat_send(gd->chat, "AT+CGEREP=?", cgerep_prefix,
+   at_cgerep_test_cb, gprs, NULL);
break;
}
  



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH 2/2] atmodem: Deactivate AT+CPSB for quectel serial modems

[Denis Kenzior]

Hi Lars,

On 8/19/20 7:13 AM, poesc...@lemonage.de wrote:

From: Lars Poeschel 

There are at the moment two quectel modems in ofono vendored as
OFONO_VENDOR_QUECTEL_SERIAL: The M95 and the MC60.
Both modems are GSM only modems, and their official documentation does
not mention the AT+CPSB command.
I have a M95 here that gives an error on issuing the AT+CPSB=1 command.
So skip this command for these two modems.
---
  drivers/atmodem/gprs.c | 1 +
  1 file changed, 1 insertion(+)



Applied, thanks.

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [PATCH v2 0/3] gemalto: USB ethernet data path for ELS81x

[Denis Kenzior]

Hi Sergey,

On 8/18/20 3:43 PM, Sergey Matyukevich wrote:

Hi Denis and all,

This patch series enables USB ethernet data path for gemalto modems
that support this feature, in particular for ELS81x devices.


All three applied (minus the S-o-B tags).  Thanks!

Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Denis Kenzior]

Hi Martin,

Instead of this dance of opening / closing the chat in order to force-cancel 
the 'AT' command queued in gemalto_enable(), maybe there should be something 
in GAtChat itself to handle this case.  






There's also the retry logic used on quectel modems with auto-detection of baud 
rates:

https://git.kernel.org/pub/scm/network/ofono/ofono.git/tree/plugins/quectel.c#n1082



Ah right, I completely forgot about g_at_chat_retry that you added for exactly 
the use case I outlined above.  Thanks for the reminder.


Maybe we should just add this modem poking logic into 
drivers/atmodem/atutil.[ch] or something if it will be used across multiple drivers.


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [RFC PATCH 4/4] plugins: gemalto: fix incomplete at-chat shutdown

[Denis Kenzior]

Hi Sergey,

On 8/15/20 4:43 PM, Sergey Matyukevich wrote:

Function gemalto_modem_ready attempts to restart AT chat data->app
after  incomplete shutdown. As a result, new AT chat does not work
as expected loosing AT commands.

Signed-off-by: Sergey Matyukevich 
---
  plugins/gemalto.c | 8 
  1 file changed, 8 insertions(+)


Patch 2, 3 look good to me.

For this one, what is it trying to do actually?



diff --git a/plugins/gemalto.c b/plugins/gemalto.c
index 238c7cc4..321c8c1b 100644
--- a/plugins/gemalto.c
+++ b/plugins/gemalto.c
@@ -222,6 +222,8 @@ static void sim_state_cb(gboolean present, gpointer 
user_data)
struct ofono_modem *modem = user_data;
struct gemalto_data *data = ofono_modem_get_data(modem);
  
+	DBG("");

+
at_util_sim_state_query_free(data->sim_state_query);
data->sim_state_query = NULL;
  
@@ -241,6 +243,8 @@ static void cfun_enable(gboolean ok, GAtResult *result, gpointer user_data)

struct ofono_modem *modem = user_data;
struct gemalto_data *data = ofono_modem_get_data(modem);
  
+	DBG("");

+
if (!ok) {
g_at_chat_unref(data->app);
data->app = NULL;
@@ -451,6 +455,8 @@ static void gemalto_modem_ready(GAtResult *result, gpointer 
user_data)
data->modem_ready_id = 0;
data->trial_cmd_id = 0;
  
+	g_at_chat_cancel_all(data->app);

+   g_at_chat_unregister_all(data->app);


These get called automatically on unref below, assuming  this is the last (it 
should be) reference.



g_at_chat_unref(data->app);


Instead of this dance of opening / closing the chat in order to force-cancel the 
'AT' command queued in gemalto_enable(), maybe there should be something in 
GAtChat itself to handle this case.  Either a 'force cancel the top of the queue 
because I know the modem is broken' or maybe some form of 'send this on the port 
until it responds properly'.  Alternatively, you could simply use GAtIO and a 
timer to write stuff to the port until it responds and only create the chat at 
that time.


There's already something a bit similar in the form of 
g_at_chat_set_wakeup_command().  It was used on the old Neo Freerunner modem 
which would go to 'sleep' after several seconds of inactivity.  So the 
subsequent AT command would get eaten.  You basically had to poke it with an 
empty command, have it timeout / return OK and then you could go on with 
submitting AT commands again.  But I don't think it is a good fit for this 
particular case.


  
  	data->app = open_device(app);

@@ -466,6 +472,8 @@ static void gemalto_at_cb(gboolean ok, GAtResult *result, 
gpointer user_data)
struct ofono_modem *modem = user_data;
struct gemalto_data *data = ofono_modem_get_data(modem);
  
+	DBG("");

+
g_at_chat_unregister(data->app, data->modem_ready_id);
data->modem_ready_id = 0;
  



Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org

Re: [RFC PATCH 1/4] drivers: gemalto: add gprs-context driver

[Denis Kenzior]

Hi Sergey,

On 8/15/20 4:43 PM, Sergey Matyukevich wrote:

Some gemalto modems provide USB ethernet interfaces for data path.
Implement gprs-context driver for such modems to send data via
USB ethernet rather than fallback to PPP.

Signed-off-by: Sergey Matyukevich 
---
  Makefile.am |   3 +-
  drivers/gemaltomodem/gemaltomodem.c |   4 +-
  drivers/gemaltomodem/gemaltomodem.h |   3 +
  drivers/gemaltomodem/gprs-context.c | 280 
  4 files changed, 288 insertions(+), 2 deletions(-)
  create mode 100644 drivers/gemaltomodem/gprs-context.c



This looks fine to me.  My only 2 nitpicks are:

1. We do not use S-o-B, so feel free to drop those in the future or I will just 
drop them when applying.



+
+static int gemalto_gprs_context_probe(struct ofono_gprs_context *gc,
+   unsigned int vendor, void *data)
+{
+   GAtChat *chat = data;
+   struct gprs_context_data *gcd;
+
+   DBG("");
+
+   gcd = g_try_new0(struct gprs_context_data, 1);
+   if (gcd == NULL)
+   return -ENOMEM;
+


2. We now prefer to use g_new0 for small structure allocations as these can't 
really fail on Linux userspace in practice.  Keeps the code shorter as well.



+   gcd->chat = g_at_chat_clone(chat);
+


Regards,
-Denis
___
ofono mailing list -- ofono@ofono.org
To unsubscribe send an email to ofono-le...@ofono.org