Re: [PATCH 1/2] sim: fix minor issue in ofono_sim_get_phase
Hi Jeevaka, On 12/06/2010 12:39 PM, Jeevaka Badrappan wrote: --- src/sim.c |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) Patch has been applied, thanks. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH 2/2] simfs: Add unknown sim phase check
Hi Jeevaka, On 12/06/2010 12:39 PM, Jeevaka Badrappan wrote: --- src/simfs.c |9 - 1 files changed, 8 insertions(+), 1 deletions(-) Patch has been applied, thanks. Regards, -Denis ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/5] Call Counters (2nd)
Hello Marcel, I'll answer to two separates mails (of this thread) in one go: On 10 Dec 2010, Marcel Holtmann wrote: Kai Vehmanen wrote: So whether this code is in oFono or elsewhere, does not matter much (to overall power consumption). The main question is of course how often the counters are synced. actually it does matter since you have no extra context switch and in addition you not accidentally wake up PA and then ofonod. You have one centralized wakeup of one thread. You cannot do this with one thread wakeup. The real-time parts implementing voice data path in pulseaudio (and ones waking up to hw interrupts) need to follow soft-realtime practises to maximize execution determinism. I.e. these threads are run with SCHED_FIFO/RR and any calls to possibly-blocking system calls must be avoided (this includes dynamic memory allocation, access to file/sokcets, etc, etc). See e.g. JACK audio server project's guidelines for client apps for a good overview of what these limitations mean in practise. Storing the call counters requires file/socket i/o, so doing this from a real-time thread is a big no-no (and if done anyways, would be a likely cause to audio hickups). So you need to wake up another thread in any case (e.g. PA has separate threads for DBus and talking to PA clients). Of course an explicit wake-up is better than relying on timer slack to coalesce the wakeup of ofonod and pulseaudio, so I agree using a PA thread is technically better and more robust way to ensure no extra wakeups occur, but if/when implemented properly, waking up ofonod really has no penalty either. But the above is really a minor issue I think. I think the most important argument against doing this in PA is (quoting my earlier reply): --cut-- - lots of modems still handle all audio and PA will know nothing about calls at all with these modems --ct--- This needs to be done in a component that is tracking calls, independently of what modem is used. And then one further comment to a more recent mail in this thread: On 12 Dec 2010, Marcel Holtmann wrote: is actually everybody in agreement here that we talk about actual time spent on the phone talking? Denis and myself clearly see it that this is a sensible approach to overall call counters. And if that is the case, then this maps closely to what PA has to track anyway. No, no. Surely what matters are the individual calls, and their state w.r.t. signaling. And this is what the patches Andras' has sent are tracking. PA on the other hand tracks the traffic channel state and that cannot be used. E.g. in common real-life cases (handovers), the traffic channel can be temporarily disconnected during an active call (PA will see this, but it is _not_ visible in either the phone UI nor in billing). For to PA to implement call counters, it would have to be extended to track individual call state. Br, -- Kai Vehmanen ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] ifx: adding Infineon modem self test
The conclusion was that these two test commands should be issued from the AP to ensure that the modem is functioning properly. I am not aware of the conclusion. My understanding is still that the modem firmware is suppose to do its selftests all by itself. It does not. So what is the reasoning behind doing these selftests from the host. It makes it easier for the host to record diagnostics / error codes this way. At the moment this is limited to recording it in the log file, as oFono doesn't provide a good way for reporting back error conditions when powering up a modem fails. Cheers, Waldo ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] ifx: adding Infineon modem self test
Hi Waldo, The conclusion was that these two test commands should be issued from the AP to ensure that the modem is functioning properly. I am not aware of the conclusion. My understanding is still that the modem firmware is suppose to do its selftests all by itself. It does not. this is really new to me. Last time this has been talked about, that was different. So what is the reasoning behind doing these selftests from the host. It makes it easier for the host to record diagnostics / error codes this way. At the moment this is limited to recording it in the log file, as oFono doesn't provide a good way for reporting back error conditions when powering up a modem fails. There is nothing more that you can provide to the host anyway. Just having one AT command to read the selftest log from the modem would have been just fine. Right now I am not seeing any advantage why the host should execute selftests. And essentially this really only makes sense once the oFono core can mark a modem as broken and then just not even try to re-enable it. We don't even have a state for this since it was never a requirement to have such a state. While Lockdown comes pretty close, it is semantically something totally different. There is also the other thing, that we have no numbers on the expected execution time of these selftests. This needs to be documented inside the code. Otherwise this gets out of control. And just to make this clear. These are run on every oFono modem enabling execution. This is different from just on initial modem power on. Are you okay with the impact on modem enabling time? Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] ifx: adding Infineon modem self test
Hi Robertino,
static const char *none_prefix[] = { NULL };
static const char *xdrv_prefix[] = { +XDRV:, NULL };
+static const char *empty_prefix[] = { , NULL };
this is essentially to just passing NULL as argument. What are you
trying to achieve here?
+static const char *modem_self_tests[][2] = {
+ { RTC Test, a...@rtc:rtc_gti_test_verify_32khz() },
+ { Version Info Test, a...@vers:device_version_id()
},
+ { NULL, NULL } };
The coding style is broken here. Please check the oFono source code for
example on how to format these type of array correctly.
In addition the string array is wrong. It should be a struct array. The
magic of [1] and [2] for picking the command or the text string is not
intuitive and leads to unreadable code.
struct ifx_data {
GIOChannel *device;
@@ -99,6 +105,8 @@ struct ifx_data {
int audio_loopback;
struct ofono_sim *sim;
gboolean have_sim;
+ guint self_test_timeout;
+ int self_test_idx;
};
static void ifx_debug(const char *str, void *user_data)
@@ -545,6 +553,82 @@ static gboolean mux_timeout_cb(gpointer user_data)
return FALSE;
}
+static gboolean self_test_timeout_cb(gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct ifx_data *data = ofono_modem_get_data(modem);
+
+ ofono_error(Timeout with modem self_test);
You need an extra empty line here. The error is logically separated.
+ g_source_remove(data-self_test_timeout);
You can not remove a source in its own callback. Have you tested this
code path actually?
+ data-self_test_timeout = 0;
+
+ shutdown_device(data);
You can not use shutdown_device here since the multiplexer is not ready
yet. If you decide the execute the selftests before bringing up the
multiplexer then this is a different error path.
You need to cleanup data-dlcs[AUX_DLC] and data-device in this failure
case.
+ ofono_modem_set_powered(modem, FALSE);
This needs an extra empty before the return statement.
+ return FALSE;
+}
+
+static void ifx_self_test_get_device_resp(GAtResult *result, int test_id)
+{
The function is just wrong in conjunction with your array approach.
+ GAtResultIter iter;
+ const char *str;
+
+ ofono_info(Modem Response: %s,
+ modem_self_tests[test_id][0]);
Can you give an example on a positive and negative selftest response
here. I don't wanna spam the logs with details if everything is fine.
This should use ofono_error and that only in case of an error.
+ g_at_result_iter_init(iter, result);
+
+ while (g_at_result_iter_next(iter, NULL)) {
+
+ if (g_at_result_iter_next_string(iter, str))
+ ofono_info(Modem Response: %s, str);
+ }
What is this while loop actually doing? How does a response look like.
+}
+
+static void ifx_self_test_cb(gboolean ok, GAtResult *result,
+ gpointer user_data)
+{
+ struct ofono_modem *modem = user_data;
+ struct ifx_data *data = ofono_modem_get_data(modem);
+
+ if (ok) {
This needs to be a if (!ok) here actually and then leaving this function
with a simple error case.
Having the positive handling nested is not a good idea and we don't
really do it that way.
+
+ if (data-self_test_timeout 0) {
+ g_source_remove(data-self_test_timeout);
+ data-self_test_timeout = 0;
+ }
+
+ ifx_self_test_get_device_resp(result, data-self_test_idx);
+ data-self_test_idx++;
+
+ if (modem_self_tests[data-self_test_idx][1] != NULL) {
+
No empty line here.
+ g_at_chat_send(data-dlcs[AUX_DLC],
+ modem_self_tests[data-self_test_idx][1],
+ empty_prefix, ifx_self_test_cb, modem, NULL);
+
+ data-self_test_timeout = g_timeout_add_seconds(10,
+ self_test_timeout_cb, modem);
+ return;
No empty line here, but this is not needed in case of (!ok) anyway.
+ } else {
+
No empty line here either.
+ /* Enable MUX Channels */
+ data-frame_size = 1509;
+ g_at_chat_send(data-dlcs[AUX_DLC],
+ AT+CMUX=0,0,,1509,10,3,30,,, NULL,
+ mux_setup_cb, modem, NULL);
+
+ data-mux_init_timeout = g_timeout_add_seconds(5,
+ mux_timeout_cb, modem);
+ return;
+ }
+ }
With proper if (error) { ... ; return; } coding style this function
becomes a lot more readable.
+
+ ofono_error(Modem %s: FAIL, modem_self_tests[data-self_test_idx][0]);
+ shutdown_device(data);
Again here, the
[PATCH] test-stkutil: Modify the check logic of time zone
--- unit/test-stkutil.c |6 +- 1 files changed, 5 insertions(+), 1 deletions(-) diff --git a/unit/test-stkutil.c b/unit/test-stkutil.c index 868d54b..666f4eb 100644 --- a/unit/test-stkutil.c +++ b/unit/test-stkutil.c @@ -245,7 +245,11 @@ static void check_gsm_sms(const struct sms *command, g_assert(ca-hour == ta-hour); g_assert(ca-minute == ta-minute); g_assert(ca-second == ta-second); - g_assert(ca-timezone == ta-timezone); + g_assert(ca-has_timezone == ta-has_timezone); + + if (ta-has_timezone) + g_assert(ca-timezone == ta-timezone); + break; } case SMS_VALIDITY_PERIOD_FORMAT_ENHANCED: -- 1.7.2.3 ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH 0/5] Call Counters (2nd)
Hello, On 11 Dec 2010, Denis-Courmont Remi wrote: let me repeat my question here. Does this suppose to be represent spent time on calls (what I called talk time) or actual billing minutes. As said earlier, this is about the talk time. The scenario is a user ejects the battery during an already long-standing call or gets the device to crash or reset, gets billed, and comes and complain to its operator that (s)he gets overcharged. The counters will be much more wrong if we do not I'm no longer sure what you guys mean with billing minutes and talk time here, so maybe we should talk in terms of code. E.g. look at the patches Andras sent to the list to see what is tracked and how: - each call is tracked separately (e.g. counter still running for held calls) - tracking separately for MO/MT calls - call starts when state goes to ACTIVE (from INCOMING/ALERTING/DIALING/WAITING) - call terminates to DISCONNECTED In case of conflict, what's actually in Andras' patches overrides my interpretation above. ;) This concept is already in oFono core (call-start_time is created when calls start, and __ofono_history_call_ended is called when calls end, matching the above). That said, I do believe the stored value has to be more precise than minutes. Otherwise per call error may add up far too quickly. Yes, but I agree with Marcel that a 60sec update interval sounds reasonable (so higher resolution for stored values, but updates at e.g. every minute). Even better if the refresh rate can be tuned. ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
