GPS deinitialization

[anicare-tero]

Hi,

OS: Windows 10
Editor: SEGGER Embedded Studio for ARM (Nordic Edition) V5.10d (64-bit)
nrfjprog version: 10.12.1
JLinkARM.dll version: 6.88a
nRF Connect SDK version: 1.4.0

I have had some problems with powering off our custom nRF9160 device. For some reason the GPS was starting again (I'm receiving GPS_EVT_SEARCH_STARTED event) even if it was stopped and that was causing a reboot for the device. Finally I got it fixed by adding a custom deinit function for nrf9160_gps driver and now our device is powering off. I have a similar gps_controller module than Asset Tracker has.

I added these:

gps_controller.c

/** @brief Deinitializes the GPS device. */
int gps_control_deinit(void)
{
	int err;

	if (gps_dev == NULL) {
		LOG_ERR("Error: Could not get %s device",
			log_strdup(CONFIG_GPS_DEV_NAME));
		return -ENODEV;
	}

	err = gps_deinit(gps_dev);
	if (err) {
		LOG_ERR("Error: Could not deinitialize GPS, error: %d", err);
		return err;
	}

	LOG_INF("GPS deinitialized");

	return err;
}

gps_controller.h

int gps_control_deinit(void);

nrf9160_gps.c

static int deinit(const struct device *dev)
{
	struct gps_drv_data *drv_data = dev->data;

	if (!atomic_get(&drv_data->is_init)) {
		LOG_WRN("GPS is already deinitialized");

		return -EALREADY;
	}

	k_thread_abort(drv_data->thread_id);

	atomic_set(&drv_data->is_init, 0);

	return 0;
}

static const struct gps_driver_api gps_api_funcs = {
...
	.deinit = deinit,
...
};

I wonder why there isn't already a deinit function for nrf9160_gps driver. I'm not sure if that's the proper way to do that but at least it's working for me. I hope you could help me to create a proper function for deinit.

Regards,
Tero

[Didrik Rokhaug]

Hi,

What modem firmware are you using?

What mode are you running the GPS in (single shot, periodic, continuous)?

What you are seeing might be periodic almanac downloads performed by the modem.

Please see this answer by one of my colleague for more information: https://devzone.nordicsemi.com/f/nordic-q-a/67618/gps-position-fix-interval/277521#277521

In your deinit function, you should also make sure that all resources allocated by the driver has been properly deallocated. In your case, it seems like you are at least leaving the socket open, though you should also check if you are leaking other resources.

Best regards,

Didrik

[anicare-tero]

Hi Didrik,

Thanks for replying!

Didrik Rokhaug said:

What modem firmware are you using?

mfw_nrf9160_1.2.2 and also tested with 1.2.1.

Didrik Rokhaug said:

What mode are you running the GPS in (single shot, periodic, continuous)?

Periodic.

Didrik Rokhaug said:

Please see this answer by one of my colleague for more information: https://devzone.nordicsemi.com/f/nordic-q-a/67618/gps-position-fix-interval/277521#277521

There Håkon Alseth said: "However, it is possible to prevent scheduled downloads by starting GNSS whenever a fix is needed and stopping it right after the fix.". I'm stopping the GPS before powering the device off so that shouldn't be the case, right?

Didrik Rokhaug said:

In your deinit function, you should also make sure that all resources allocated by the driver has been properly deallocated. In your case, it seems like you are at least leaving the socket open, though you should also check if you are leaking other resources.

Is there coming a deinit function from your side? And I'm gonna update my function to at least close the socket too. What are the other resources that should be deallocated? And how should I do that?

EDIT: I updated the nrf9160_gps.c a little bit. Should it be better now and what else should I do?

static int deinit(const struct device *dev)
{
	int err;
	struct gps_drv_data *drv_data = dev->data;

	if (!atomic_get(&drv_data->is_init)) {
		LOG_WRN("GPS is already deinitialized");

		return -EALREADY;
	}

	cancel_works(drv_data);
	atomic_set(&drv_data->is_active, 0);
	atomic_set(&drv_data->is_shutdown, 1);

	LOG_DBG("Closing GPS socket, fd: %d", drv_data->socket);
	err = nrf_close(drv_data->socket);
	if (err < 0) {
		LOG_ERR("Failed to close GPS socket, err: %d", err);
	}

	LOG_DBG("Aborting GPS thread.");
	k_thread_abort(drv_data->thread_id);

	atomic_set(&drv_data->is_init, 0);

	return 0;
}

Best Regards,
Tero

[Didrik Rokhaug]

Hi, and sorry for the late answer.

Your deinit functions seems fine.

I am however not sure about drv_data->thread_run_sem. For the most part, it seems OK to initialize a semaphore twice (I assume you are calling the init() function to re-initialize the driver?), but there are a few calls inside k_sem_init() I haven't been able to check.

It might be that everything works OK, I just don't know for sure.

However, I did a quick test with a simple application calling k_sem_init() on the same semaphore in a loop, and it has currently been running for over 9 300 000 iterations.

My test doesn't test any usage of the semaphore though, so I can not guarantee that everything is 100% fine. But at least there doesn't seem to be any major memory leaks.

[anicare-tero]

Hi Didrik,

Didrik Rokhaug said:

I assume you are calling the init() function to re-initialize the driver?

I'm not re-initializing the driver because I'm only doing deinit before powering the device off. And if we power on the device again, it should do a normal startup and initialize the GPS driver normally with the default init function.

Didrik Rokhaug said:

Your deinit functions seems fine.

They have worked fine for me so for now I'm going with those. Do you have any information if there are coming the deinit functions from your side?

[Didrik Rokhaug]

anicare-tero said:

Do you have any information if there are coming the deinit functions from your side?

 We do not have any plans for implementing a deinit function for the GPS driver.

However, the current GPS driver will soon be deprecated, and replaced with a (similar) API in libmodem (formerly bsdlib).

[anicare-tero]

Okay, that's good to know. I'm going with my custom deinit function for now and then take the API to use when it's replaces the GPS driver.

Thanks for your support!