Qtopia-interest Archive, April 2008
Funny issues with QSimInfo, ASSERTS and ficgta01
Message 1 in thread
Hey,
I have been quite the last days, not because I run out of issues, but because
the feedback is so low.
Anyway this bug was rather funny and teaches us why Copy and Paste is
certainly a bad idea.
Setting: I have this SIM PIN Dialog showing on my X11 setup, after a while it
vanished.
I just decided to look at the reason why it vanishes, oh yeah, I hit an ASSERT
in CellModemManager::simNotInserted. I have already written on my attitude
regarding asserts (I think glib, at least from the concept, is quite good
there). So it asked for my SIM PIN and then decided I have no SIM and
crashed. *weird*
I started to look where notInserted is coming from and found QSimInfo and
QModemSimInfo. I saw that QModemSimInfo is emitting the notInserted signal in
two conditions. It checks if it needs a SIM PIN and if it doesn't and CIMI
failed too often it emits this signal. This simpinrequired and comes from the
QModemPinManager and this code looks reasonable as well (there is one
ocassion where this message is not posted, but it might be dead code...).
So QModemSimInfo tries to check for simpinrequired and still notInserted is
emitted. Setting a breakpoint QModemSimInfo shows it was not initiated.
*weird*. Grepped in the whole treee (not only src/libraries) for notInserted
and found a sucker in the ficgta01 directory.
Ah indeed a direct copy of QModemSimInfo, the only difference is that
QModemSimInfo honors/listens to the simpinrequired post your FicGta01
implementation doesn't.
A patch is attached to use QModemSimInfo and kill the code in the ficgta01
vendor plugin.
Things to learn:
1. Do not copy and paste such things (also remove your four copies of yuv
color blending and move one into Qt, I think QColor gained one?)
2. If you copy and paste such code. Update all occurences when you update
one.
3. I don't know the reason why this was copy and pasted but if you think you
need to copy and paste whole classes for a little customisation you got
the abstraction wrong. You don't want to copy the class, you want to fix
the abstraction and then reimplement the base.
feedback welcome
z.
commit 4d6624ea3f0e6c2c46ea853fc9b715a33eac91ad
commit 4d6624ea3f0e6c2c46ea853fc9b715a33eac91ad
Author: Holger Freyther <zecke@xxxxxxxxxxx>
Date: Tue Apr 1 21:39:06 2008 +0200
ti/calypso/fic: No use to implement a custom Ficgta01SimInfo/QSimInfo
Ficgta01SimInfo and QModemSimInfo will win any look a like competition. The
only difference is that Ficgta01SimInfo will report/emit notInserted after
a couple of failed CIMI requests even if we are currently waiting for the
SIM PIN (so we know we have a SIM). QModemSimInfo knows that we query for
the SIM PIN and is not saying we don't have a SIM PIN.
Why is this important? With the copy and pasted code we hit the assert in
CellModemManager::simNotInserted(). With QModemInfo we don't... this makes
me wonder what other joys are hidden in vendor_ficgta01.
diff --git a/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp b/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp
index a86bae9..41819e5 100644
--- a/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp
+++ b/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp
@@ -561,7 +561,7 @@ void Ficgta01ModemService::initialize()
addInterface( new Ficgta01BandSelection( this ) );
if ( !supports<QSimInfo>() )
- addInterface( new Ficgta01SimInfo( this ) );
+ addInterface( new QModemSimInfo( this ) );
if ( !callProvider() )
setCallProvider( new Ficgta01CallProvider( this ) );
@@ -826,100 +826,6 @@ void Ficgta01CallVolume::setMicVolumeRange(int min,int max)
}
-// Number of milliseconds between polling attempts on AT+CIMI command.
-#ifndef CIMI_TIMEOUT
-#define CIMI_TIMEOUT 2000
-#endif
-
-class Ficgta01SimInfoPrivate
-{
-public:
- Ficgta01ModemService *service;
- QTimer *checkTimer;
- int count;
-};
-
-Ficgta01SimInfo::Ficgta01SimInfo( Ficgta01ModemService *service )
- : QSimInfo( service->service(), service, QCommInterface::Server )
-{
- d = new Ficgta01SimInfoPrivate();
- d->service = service;
- d->checkTimer = new QTimer( this );
- d->checkTimer->setSingleShot( true );
- connect( d->checkTimer, SIGNAL(timeout()), this, SLOT(requestIdentity()) );
- d->count = 0;
-
-
- // Perform an initial AT+CIMI request to get the SIM identity.
- QTimer::singleShot( 0, this, SLOT(requestIdentity()) );
-}
-
-Ficgta01SimInfo::~Ficgta01SimInfo()
-{
- delete d;
-}
-
-void Ficgta01SimInfo::simInserted()
-{
- if ( !d->checkTimer->isActive() )
- requestIdentity();
-}
-
-void Ficgta01SimInfo::simRemoved()
-{
- setIdentity( QString() );
-}
-
-void Ficgta01SimInfo::requestIdentity()
-{
- d->service->primaryAtChat()->chat
- ( "AT+CIMI", this, SLOT(cimi(bool,QAtResult)) );
-}
-
-void Ficgta01SimInfo::cimi( bool ok, const QAtResult& result )
-{
- QString id = extractIdentity( result.content().trimmed() );
- if ( ok && !id.isEmpty() ) {
- // We have a valid SIM identity.
- setIdentity( id );
- } else {
- // No SIM identity, so poll again in a few seconds for the first two minutes.
- setIdentity( QString() );
-
- if ( d->count < 120000/CIMI_TIMEOUT ) {
- d->checkTimer->start( CIMI_TIMEOUT );
- d->count++;
- } else {
- d->count = 0;
- // post a message to modem service to stop SIM PIN polling
- d->service->post( "simnotinserted" );
- emit notInserted();
- }
- // If we got a definite "not inserted" or "sim failure" error,
- // then emit notInserted().
- if ( result.resultCode() == QAtResult::SimNotInserted
- || result.resultCode() == QAtResult::SimFailure)
- emit notInserted();
- }
-}
-
-// Extract the identity information from the content of an AT+CIMI response.
-// It is possible that we got multiple lines, including some unsolicited
-// notifications from the modem that are not yet recognised. Skip over
-// such garbage and find the actual identity.
-QString Ficgta01SimInfo::extractIdentity( const QString& content )
-{
- QStringList lines = content.split( QChar('\n') );
- foreach ( QString line, lines ) {
- if ( line.length() > 0 ) {
- uint ch = line[0].unicode();
- if ( ch >= '0' && ch <= '9' )
- return line;
- }
- }
- return QString();
-}
-
Ficgta01PreferredNetworkOperators::Ficgta01PreferredNetworkOperators( QModemService *service )
: QModemPreferredNetworkOperators( service )
{
diff --git a/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01_p.h b/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01_p.h
index 91cf9d2..296399c 100644
--- a/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01_p.h
+++ b/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01_p.h
@@ -188,29 +188,6 @@ private:
};
-class Ficgta01SimInfoPrivate;
-
-class Ficgta01SimInfo : public QSimInfo
-{
- Q_OBJECT
-public:
- Ficgta01SimInfo(Ficgta01ModemService *service );
- ~Ficgta01SimInfo();
-
-protected slots:
- void simInserted();
- void simRemoved();
-
-private slots:
- void requestIdentity();
- void cimi( bool ok, const QAtResult& result );
-
-private:
- Ficgta01SimInfoPrivate *d;
-
- static QString extractIdentity( const QString& content );
-};
-
class Ficgta01PreferredNetworkOperators : public QModemPreferredNetworkOperators
{
Q_OBJECT
Message 2 in thread
Holger Freyther wrote:
> Hey,
>
> I have been quite the last days, not because I run out of issues, but because
> the feedback is so low.
>
> Anyway this bug was rather funny and teaches us why Copy and Paste is
> certainly a bad idea.
>
> Setting: I have this SIM PIN Dialog showing on my X11 setup, after a while it
> vanished.
>
> I just decided to look at the reason why it vanishes, oh yeah, I hit an ASSERT
> in CellModemManager::simNotInserted. I have already written on my attitude
> regarding asserts (I think glib, at least from the concept, is quite good
> there). So it asked for my SIM PIN and then decided I have no SIM and
> crashed. *weird*
>
> I started to look where notInserted is coming from and found QSimInfo and
> QModemSimInfo. I saw that QModemSimInfo is emitting the notInserted signal in
> two conditions. It checks if it needs a SIM PIN and if it doesn't and CIMI
> failed too often it emits this signal. This simpinrequired and comes from the
> QModemPinManager and this code looks reasonable as well (there is one
> ocassion where this message is not posted, but it might be dead code...).
>
> So QModemSimInfo tries to check for simpinrequired and still notInserted is
> emitted. Setting a breakpoint QModemSimInfo shows it was not initiated.
> *weird*. Grepped in the whole treee (not only src/libraries) for notInserted
> and found a sucker in the ficgta01 directory.
>
> Ah indeed a direct copy of QModemSimInfo, the only difference is that
> QModemSimInfo honors/listens to the simpinrequired post your FicGta01
> implementation doesn't.
>
> A patch is attached to use QModemSimInfo and kill the code in the ficgta01
> vendor plugin.
>
>
> Things to learn:
> 1. Do not copy and paste such things (also remove your four copies of yuv
> color blending and move one into Qt, I think QColor gained one?)
There are only certain things we can move into Qt.
>
> 2. If you copy and paste such code. Update all occurences when you update
> one.
>
> 3. I don't know the reason why this was copy and pasted but if you think you
> need to copy and paste whole classes for a little customisation you got
> the abstraction wrong. You don't want to copy the class, you want to fix
> the abstraction and then reimplement the base.
The comment to the submit is this:
"Move the SimFailure logic into the Neo modem vendor plugin
because it seems to be interfering with the Greenphone"
The reason for this, basically is because the Neo's modem seems to only respond simfailure when its
not inserted, from what I remember of it, and adding the || result.resultCode() ==
QAtResult::SimFailure line to the main library caused problems on the Greenphone.
Which is why it is in the fic modem plugin.
>
>
> feedback welcome
>
> z.
>
--
[ signature omitted ]
Message 3 in thread
On Wednesday 02 April 2008 00:05:08 Lorn Potter wrote:
> > 2. If you copy and paste such code. Update all occurences when you
> > update one.
> >
> > 3. I don't know the reason why this was copy and pasted but if you think
> > you need to copy and paste whole classes for a little customisation you
> > got the abstraction wrong. You don't want to copy the class, you want to
> > fix the abstraction and then reimplement the base.
>
> The comment to the submit is this:
>
> "Move the SimFailure logic into the Neo modem vendor plugin
> because it seems to be interfering with the Greenphone"
>
> The reason for this, basically is because the Neo's modem seems to only
> respond simfailure when its not inserted, from what I remember of it, and
> adding the || result.resultCode() == QAtResult::SimFailure line to the main
> library caused problems on the Greenphone. Which is why it is in the fic
> modem plugin.
So the checking for the simpin was lately afterwards? In this case the 2nd
statement applies.
But still where is the rationale. You need to adjust the behaviour of one
method for your hardware to work correctly. Is Qtopia's answer to "Create
more, code less" using copy and paste?
anyway thanks for telling me the commit message, I would have missed this
additional condition otherwise.
thanks
--
[ signature omitted ]
Message 4 in thread
On Wednesday 02 April 2008 01:51:53 Holger Freyther wrote:
> On Wednesday 02 April 2008 00:05:08 Lorn Potter wrote:
> > > 2. If you copy and paste such code. Update all occurences when you
> > > update one.
> > >
> > > 3. I don't know the reason why this was copy and pasted but if you
> > > think you need to copy and paste whole classes for a little
> > > customisation you got the abstraction wrong. You don't want to copy the
> > > class, you want to fix the abstraction and then reimplement the base.
> >
> > The comment to the submit is this:
> >
> > "Move the SimFailure logic into the Neo modem vendor plugin
> > because it seems to be interfering with the Greenphone"
> >
> > The reason for this, basically is because the Neo's modem seems to only
> > respond simfailure when its not inserted, from what I remember of it, and
> > adding the || result.resultCode() == QAtResult::SimFailure line to the
> > main library caused problems on the Greenphone. Which is why it is in the
> > fic modem plugin.
>
> So the checking for the simpin was lately afterwards? In this case the 2nd
> statement applies.
> But still where is the rationale. You need to adjust the behaviour of one
> method for your hardware to work correctly. Is Qtopia's answer to "Create
> more, code less" using copy and paste?
Why was copy and paste preferred over a solution like the one attached (I
might got the logic wrong, it is untested..)?
Benefits:
- Code sharing with QModemInfo
- Easy to use API
- QModemInfo stays maintainable, add new enum values, no #ifdefs in
the implementation.
regards
the curious z.
diff --git a/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp b/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp
diff --git a/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp b/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp
index 41819e5..f174393 100644
--- a/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp
+++ b/devices/ficgta01/src/plugins/phonevendors/ficgta01/vendor_ficgta01.cpp
@@ -560,8 +560,11 @@ void Ficgta01ModemService::initialize()
if ( !supports<QBandSelection>() )
addInterface( new Ficgta01BandSelection( this ) );
- if ( !supports<QSimInfo>() )
- addInterface( new QModemSimInfo( this ) );
+ if ( !supports<QSimInfo>() ) {
+ QModemSimInfo* simInfo = new QModemSimInfo( this );
+ simInfo->setSimNotInsertedReason( QModemSimInfo::Reason_SimFailure );
+ addInterface( simInfo );
+ }
if ( !callProvider() )
setCallProvider( new Ficgta01CallProvider( this ) );
diff --git a/src/libraries/qtopiaphonemodem/qmodemsiminfo.cpp b/src/libraries/qtopiaphonemodem/qmodemsiminfo.cpp
index 69c8ec1..9fb959f 100644
--- a/src/libraries/qtopiaphonemodem/qmodemsiminfo.cpp
+++ b/src/libraries/qtopiaphonemodem/qmodemsiminfo.cpp
@@ -51,6 +51,7 @@ public:
QTimer *checkTimer;
int count;
bool simPinRequired;
+ unsigned reasons : 1;
};
/*!
@@ -66,6 +67,7 @@ QModemSimInfo::QModemSimInfo( QModemService *service )
connect( d->checkTimer, SIGNAL(timeout()), this, SLOT(requestIdentity()) );
d->count = 0;
d->simPinRequired = false;
+ d->reasons = 0;
// Perform an initial AT+CIMI request to get the SIM identity.
QTimer::singleShot( 0, this, SLOT(requestIdentity()) );
@@ -140,7 +142,8 @@ void QModemSimInfo::cimi( bool ok, const QAtResult& result )
}
}
// If we got a definite "not inserted" error, then emit notInserted().
- if ( result.resultCode() == QAtResult::SimNotInserted )
+ if ( result.resultCode() == QAtResult::SimNotInserted
+ || (d->reasons & Reason_SimFailure && result.resultCode() == QAtResult::SimFailure))
emit notInserted();
}
}
@@ -153,6 +156,11 @@ void QModemSimInfo::serviceItemPosted( const QString &item )
d->simPinRequired = false;
}
+void QModemSimInfo::setSimNotInsertedReason(enum SimNotInsertedReasons reason)
+{
+ d->reasons |= reason;
+}
+
// Extract the identity information from the content of an AT+CIMI response.
// It is possible that we got multiple lines, including some unsolicited
// notifications from the modem that are not yet recognised. Skip over
diff --git a/src/libraries/qtopiaphonemodem/qmodemsiminfo.h b/src/libraries/qtopiaphonemodem/qmodemsiminfo.h
index 34c556a..01b8100 100644
--- a/src/libraries/qtopiaphonemodem/qmodemsiminfo.h
+++ b/src/libraries/qtopiaphonemodem/qmodemsiminfo.h
@@ -32,9 +32,15 @@ class QTOPIAPHONEMODEM_EXPORT QModemSimInfo : public QSimInfo
{
Q_OBJECT
public:
+ enum SimNotInsertedReasons {
+ Reason_SimFailure = 0x1
+ };
+
explicit QModemSimInfo( QModemService *service );
~QModemSimInfo();
+ void setSimNotInsertedReason(enum SimNotInsertedReasons);
+
protected slots:
void simInserted();
void simRemoved();
