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
RE: [PATCH] ifx: adding Infineon modem self test
Adding Infineon modem selftest to the plugin. It executes couple of AT commands, and based on the responses, it continues with ifx modem enabling or powers the modem down. didn't we conclude that the modem should do all these by itself? My understanding is that the modem executes these. And not that we have to trigger them. The conclusion was that these two test commands should be issued from the AP to ensure that the modem is functioning properly. Cheers, Waldo ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
RE: [PATCH] ifx: adding Infineon modem self test
Hi Waldo, Adding Infineon modem selftest to the plugin. It executes couple of AT commands, and based on the responses, it continues with ifx modem enabling or powers the modem down. didn't we conclude that the modem should do all these by itself? My understanding is that the modem executes these. And not that we have to trigger them. 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. So what is the reasoning behind doing these selftests from the host. Right now I am failing to see the benefit of these. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
Re: [PATCH] ifx: adding Infineon modem self test
Hi Robertino, Adding Infineon modem selftest to the plugin. It executes couple of AT commands, and based on the responses, it continues with ifx modem enabling or powers the modem down. didn't we conclude that the modem should do all these by itself? My understanding is that the modem executes these. And not that we have to trigger them. Regards Marcel ___ ofono mailing list ofono@ofono.org http://lists.ofono.org/listinfo/ofono
