* vendor models and options @ 2018-09-07 8:09 Giacinto Cifelli 2018-09-07 15:09 ` Denis Kenzior 0 siblings, 1 reply; 6+ messages in thread From: Giacinto Cifelli @ 2018-09-07 8:09 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 947 bytes --] Dear Denis, all, while preparing the Gemalto driver, I see that there is a potential shortcoming with the vendor structure in the atmodem. I can set OFONO_VENDOR_GEMALTO_model, but this could be a very long list for all options to maintain. I see the tendency also for other manufacturers. Would it be ok if I convert the vendor integer in a structure with model and flags? This would make the current code more compact and clearer. The alternative of cloning the atmodem is less tempting, because then it needs constant monitoring of the atmodem to porting the features in it, or conversely missing features in this general driver. One example is for the indicators +CGREG/+CEREG/+C5GREG that we are adding in atmodem (gprs.c), instead of the unique +CGREG that doesn't work anymore for LTE (in the 27.007). If I add it in Gemalto driver, it will be missing in the atmodem. thank you for any commments. Regards, Giacinto [-- Attachment #2: attachment.html --] [-- Type: text/html, Size: 1094 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: vendor models and options 2018-09-07 8:09 vendor models and options Giacinto Cifelli @ 2018-09-07 15:09 ` Denis Kenzior 2018-09-14 11:48 ` Giacinto Cifelli 0 siblings, 1 reply; 6+ messages in thread From: Denis Kenzior @ 2018-09-07 15:09 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1774 bytes --] Hi Giacinto, On 09/07/2018 03:09 AM, Giacinto Cifelli wrote: > Dear Denis, all, > > while preparing the Gemalto driver, I see that there is a potential > shortcoming with the vendor structure in the atmodem. I can set > OFONO_VENDOR_GEMALTO_model, but this could be a very long list for all > options to maintain. I see the tendency also for other manufacturers. > Would it be ok if I convert the vendor integer in a structure with model > and flags? > This would make the current code more compact and clearer. Feel free to suggest something more concrete illustrating how your proposal would result in more compact code. It seems to me that it would be quite disruptive, but I always try to have an open mind :) But of course there are other 'creative' ways we can utilize to handle this as well. > > The alternative of cloning the atmodem is less tempting, because then it > needs constant monitoring of the atmodem to porting the features in it, > or conversely missing features in this general driver. One example is > for the indicators +CGREG/+CEREG/+C5GREG that we are adding in atmodem > (gprs.c), instead of the unique +CGREG that doesn't work anymore for LTE > (in the 27.007). If I add it in Gemalto driver, it will be missing in > the atmodem. Aren't these commands now standardized? Why do you need sub-model information to handle this? Either way, it is quite hard to give you any suggestions without seeing concretely what you're trying to do. Perhaps you want to submit what you have (or some subset that illustrates the issues you're having) to the mailing list (even if it isn't completely ready, just tag it as 'RFC') and then I can give you more concrete feedback. Regards, -Denis ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: vendor models and options 2018-09-07 15:09 ` Denis Kenzior @ 2018-09-14 11:48 ` Giacinto Cifelli 2018-09-14 6:00 ` Denis Kenzior 0 siblings, 1 reply; 6+ messages in thread From: Giacinto Cifelli @ 2018-09-14 11:48 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 4378 bytes --] hi Denis, this is the structure I would like to propose: =========== in drivers/atmodem/vendor.h: enum ofono_vendor { OFONO_VENDOR_GENERIC = 0, OFONO_VENDOR_CALYPSO, OFONO_VENDOR_IFX, OFONO_VENDOR_STE, OFONO_VENDOR_MBM, OFONO_VENDOR_GOBI, OFONO_VENDOR_QUALCOMM, OFONO_VENDOR_HSO, OFONO_VENDOR_ZTE, OFONO_VENDOR_HUAWEI, OFONO_VENDOR_SIERRA, OFONO_VENDOR_NOVATEL, OFONO_VENDOR_WAVECOM, OFONO_VENDOR_NOKIA, OFONO_VENDOR_PHONESIM, OFONO_VENDOR_TELIT, OFONO_VENDOR_SPEEDUP, OFONO_VENDOR_SAMSUNG, OFONO_VENDOR_SIMCOM, OFONO_VENDOR_ICERA, OFONO_VENDOR_ALCATEL, OFONO_VENDOR_QUECTEL, OFONO_VENDOR_UBLOX, OFONO_VENDOR_GEMALTO, OFONO_VENDOR_XMM, }; enum gemalto_auth_type { OV_GEMALTO_AUTH_CGAUTH = 0, OV_GEMALTO_AUTH_SGAUTH_PWD_USER = 1, OV_GEMALTO_AUTH_SGAUTH_USER_PWD = 2, }; struct ov_gemalto { enum gemalto_auth_type auth; gboolean vts_with_quotes; }; struct ov_quectel { gboolean use_qtrpin; }; struct ov_simcom { gboolean use_atd99; }; struct ov_ublox { gboolean toby_l2; }; struct ov_wavecom { gboolean short_cpms; }; struct ofono_vendor_parameters { enum ofono_vendor vendor; unsigned int model; union { struct ov_gemalto gemalto; struct ov_quectel quelctel; struct ov_simcom simcom; struct ov_ublox ublox; struct ov_wavecom wavecom; }; }; extern struct ofono_vendor_parameters ov_generic; =========== in drivers/atmodem/atmodem.c: struct ofono_vendor_parameters ov_generic = { OFONO_VENDOR_GENERIC, 0 }; ========== and then change the *_create() functions to use 'struct ofono_vendor_parameters *' instead of 'unsigned int vendor', with default &ov_generic. In the example above, I have shown differences for the various Gemalto authentication commands (3 options), that cross with the options for VTS. And then most of the settings are common for Gemalto models (currently factored with the vendor=OFONO_VENDOR_CINTERION). If tomorrow I have to declare models for these simple settings, I have to add at least 6 with the current system, and having a full driver just because of these two AT commands seems needless. The code would be simplified because in general in a switch there will be only 1 vendor, with the current system there would be 6. Is it ok if I update the code with this vendor system? I will have to submit the entire tree, not by subdirectories, to have a consistent and compiling commit. Regards, Giacinto On Fri, Sep 7, 2018 at 5:09 PM Denis Kenzior <denkenz@gmail.com> wrote: > Hi Giacinto, > > On 09/07/2018 03:09 AM, Giacinto Cifelli wrote: > > Dear Denis, all, > > > > while preparing the Gemalto driver, I see that there is a potential > > shortcoming with the vendor structure in the atmodem. I can set > > OFONO_VENDOR_GEMALTO_model, but this could be a very long list for all > > options to maintain. I see the tendency also for other manufacturers. > > Would it be ok if I convert the vendor integer in a structure with model > > and flags? > > This would make the current code more compact and clearer. > > Feel free to suggest something more concrete illustrating how your > proposal would result in more compact code. It seems to me that it > would be quite disruptive, but I always try to have an open mind :) > > But of course there are other 'creative' ways we can utilize to handle > this as well. > > > > > The alternative of cloning the atmodem is less tempting, because then it > > needs constant monitoring of the atmodem to porting the features in it, > > or conversely missing features in this general driver. One example is > > for the indicators +CGREG/+CEREG/+C5GREG that we are adding in atmodem > > (gprs.c), instead of the unique +CGREG that doesn't work anymore for LTE > > (in the 27.007). If I add it in Gemalto driver, it will be missing in > > the atmodem. > > Aren't these commands now standardized? Why do you need sub-model > information to handle this? > > Either way, it is quite hard to give you any suggestions without seeing > concretely what you're trying to do. Perhaps you want to submit what > you have (or some subset that illustrates the issues you're having) to > the mailing list (even if it isn't completely ready, just tag it as > 'RFC') and then I can give you more concrete feedback. > > Regards, > -Denis > [-- Attachment #2: attachment.html --] [-- Type: text/html, Size: 7909 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: vendor models and options 2018-09-14 11:48 ` Giacinto Cifelli @ 2018-09-14 6:00 ` Denis Kenzior 2018-09-15 6:23 ` Giacinto Cifelli 0 siblings, 1 reply; 6+ messages in thread From: Denis Kenzior @ 2018-09-14 6:00 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 2798 bytes --] Hi Giacinto, So first of all, please stop top-posting on this mailing list. All comments should be in-line. > enum gemalto_auth_type { > OV_GEMALTO_AUTH_CGAUTH = 0, CGAUTH is a release 11 feature and should be simply probed for in the gprs-context initialization procedure, similar to how we test for +CGDATA support. > OV_GEMALTO_AUTH_SGAUTH_PWD_USER = 1, > OV_GEMALTO_AUTH_SGAUTH_USER_PWD = 2, You will need to explain the difference between SGAUTH 1 & 2 to me. Is it the parameter order that is different between firmware versions? Can't these simply be queried properly using AT^SGAUTH=? > }; So are you introducing a new gprs-context driver for gemalto or using these for the 'atmodem' driver? > > struct ov_gemalto { > enum gemalto_auth_type auth; > gboolean vts_with_quotes; Sheesh, your firmware guys somehow managed to screw up the VTS command syntax? That command has been in there for 20 years... Anyhow, I strongly discourage you from modifying the atmodem voicecall.c driver. Every vendor should have voice call state reporting commands that are far superior to the AT+CLCC polling we do in the 'atmodem' voicecall.c driver. So it should be fairly trivial to probe for +VTS syntax there. Worst case you can use ofono_modem_set_* family to set a particular modem specific property which can be available to all atom drivers via ofono_modem_get_*. So this VTS syntax can be handled that way. > }; > <snip> > > and then change the *_create() functions to use 'struct > ofono_vendor_parameters *' instead of 'unsigned int vendor', with > default &ov_generic. > > In the example above, I have shown differences for the various Gemalto > authentication commands (3 options), that cross with the options for VTS. > And then most of the settings are common for Gemalto models (currently > factored with the vendor=OFONO_VENDOR_CINTERION). > If tomorrow I have to declare models for these simple settings, I have > to add at least 6 with the current system, and having a full driver just > because of these two AT commands seems needless. No, this is all completely unneeded as far as I'm concerned. If you want to use +CGAUTH, then just probe it properly and don't pass a vendor at all. The VTS stuff I already covered. > > The code would be simplified because in general in a switch there will > be only 1 vendor, with the current system there would be 6. > > Is it ok if I update the code with this vendor system? I will have to > submit the entire tree, not by subdirectories, to have a consistent and > compiling commit. > Right now I'm completely unconvinced that such an invasive change is warranted. So no, please don't do this. Regards, -Denis ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: vendor models and options 2018-09-14 6:00 ` Denis Kenzior @ 2018-09-15 6:23 ` Giacinto Cifelli 2018-09-15 20:52 ` Denis Kenzior 0 siblings, 1 reply; 6+ messages in thread From: Giacinto Cifelli @ 2018-09-15 6:23 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 5122 bytes --] Hi Denis, On Fri, Sep 14, 2018 at 6:01 PM Denis Kenzior <denkenz@gmail.com> wrote: > Hi Giacinto, > > So first of all, please stop top-posting on this mailing list. All > comments should be in-line. > > understood. > > enum gemalto_auth_type { > > OV_GEMALTO_AUTH_CGAUTH = 0, > > CGAUTH is a release 11 feature and should be simply probed for in the > gprs-context initialization procedure, similar to how we test for > +CGDATA support. > > OV_GEMALTO_AUTH_SGAUTH_PWD_USER = 1, > > OV_GEMALTO_AUTH_SGAUTH_USER_PWD = 2, > > You will need to explain the difference between SGAUTH 1 & 2 to me. Is > it the parameter order that is different between firmware versions? > Can't these simply be queried properly using AT^SGAUTH=? > > > }; > > yes, CGAUTH can be queried, but there is no fallback command in case it fails, so it has to be pre-defined based on the model used. No query commands would not return the name of the parameters, which in this case is the only difference between username and password. > So are you introducing a new gprs-context driver for gemalto or using > these for the 'atmodem' driver? > > I am introducing a specific gprs-context for some models, using atmodem for others, and mbim and qmi for other models. I try to stick to gprs (not gprs-context) for most of them, because this is pretty much the same for most of them. For some models that have a lot of differences we are going to use specific atoms. We use the same code for the lte atom as well, unless there is a way to factorize it. Just to explain the target, overall we are introducing support for more than 50 models (not firmware versions, plain different models). Historically, an entire family of models share the same USB enumeration, despite the differences in hardware and software (for example for voice support), so there will be about two dozen USB PID, but more models behind. > > > struct ov_gemalto { > > enum gemalto_auth_type auth; > > gboolean vts_with_quotes; > > Sheesh, your firmware guys somehow managed to screw up the VTS command > syntax? That command has been in there for 20 years... > and our Siemens/Cinterion/Gemalto from earlier than that. For backward compatibility, model after model, we kept the pre-standard syntax in some models, and introduced the standard one in other models whenever possible. > > Anyhow, I strongly discourage you from modifying the atmodem voicecall.c > driver. Every vendor should have voice call state reporting commands > that are far superior to the AT+CLCC polling we do in the 'atmodem' > voicecall.c driver. So it should be fairly trivial to probe for +VTS > syntax there. > > we already discussed this. We do plan to have our own voicecall.c using URCs instead of polling, among the rest. We planned to do it a bit later, but I think you are right, we can already clone the atmodem/voicecall for this change, then introducing the rest of the changes later. > Worst case you can use ofono_modem_set_* family to set a particular > modem specific property which can be available to all atom drivers via > ofono_modem_get_*. So this VTS syntax can be handled that way. > > > }; > > > > I think ofono_modem_set_*/ofono_modem_get_* is just what we need for the two examples above (and others). > <snip> > > > > > and then change the *_create() functions to use 'struct > > ofono_vendor_parameters *' instead of 'unsigned int vendor', with > > default &ov_generic. > > > > In the example above, I have shown differences for the various Gemalto > > authentication commands (3 options), that cross with the options for > VTS. > > And then most of the settings are common for Gemalto models (currently > > factored with the vendor=OFONO_VENDOR_CINTERION). > > If tomorrow I have to declare models for these simple settings, I have > > to add at least 6 with the current system, and having a full driver just > > because of these two AT commands seems needless. > > No, this is all completely unneeded as far as I'm concerned. If you > want to use +CGAUTH, then just probe it properly and don't pass a vendor > at all. The VTS stuff I already covered. > > well, the point is that we need to use CGAUTH for some models, AND the rest of the code selected with OFONO_VENDOR_CINTERION within the same atom. But the aforementioned ofono_modem_set_*/ofono_modem_get_* is the mechanism that we need. > > > > The code would be simplified because in general in a switch there will > > be only 1 vendor, with the current system there would be 6. > > > > Is it ok if I update the code with this vendor system? I will have to > > submit the entire tree, not by subdirectories, to have a consistent and > > compiling commit. > > > > Right now I'm completely unconvinced that such an invasive change is > warranted. So no, please don't do this. > After your comments, I am convinced as well that we don't need this. You were also right that with an example we could explain each other better. > > Regards, > -Denis > [-- Attachment #2: attachment.html --] [-- Type: text/html, Size: 7180 bytes --] ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: vendor models and options 2018-09-15 6:23 ` Giacinto Cifelli @ 2018-09-15 20:52 ` Denis Kenzior 0 siblings, 0 replies; 6+ messages in thread From: Denis Kenzior @ 2018-09-15 20:52 UTC (permalink / raw) To: ofono [-- Attachment #1: Type: text/plain, Size: 1599 bytes --] Hi Giacinto, > yes, CGAUTH can be queried, but there is no fallback command in case it > fails, so it has to be pre-defined based on the model used. You might have to show me more specifics. I don't see why we couldn't query +CGAUTH=? in all cases and if it isn't supported, fall back to vendor specific commands? > No query commands would not return the name of the parameters, which in > this case is the only difference between username and password. That's too bad. Some vendors define a different length for username / password, so you could guess the order by which lengths are returned. Others use weird strings with password or username in the '=?' response. However, since you likely want to use a vendor specific gprs-context driver anyway, this really shouldn't be that big of a deal. > > I am introducing a specific gprs-context for some models, using atmodem > for others, and mbim and qmi for other models. > I try to stick to gprs (not gprs-context) for most of them, because this > is pretty much the same for most of them. For some models that have a > lot of differences we are going to use specific atoms. > We use the same code for the lte atom as well, unless there is a way to > factorize it. It is generally quite easy to share things between AT command based drivers. See drivers/atmodem/atutil.[ch]. Also, you might want to start by submitting a small subset of changes you have pending as a trial run to get comfortable with the overall development process, patch reviews, resubmissions, etc. Regards, -Denis ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2018-09-15 20:52 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-09-07 8:09 vendor models and options Giacinto Cifelli 2018-09-07 15:09 ` Denis Kenzior 2018-09-14 11:48 ` Giacinto Cifelli 2018-09-14 6:00 ` Denis Kenzior 2018-09-15 6:23 ` Giacinto Cifelli 2018-09-15 20:52 ` Denis Kenzior
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.