Add support for reading NMEA sentences from I2C

[alistair23]

Pull Request Overview

NMEA is a common output from GNSS devices. It is a "sentence" that starts with a $.

This adds support for userspace to read NMEA sentences from a GNSS device connected via I2C.

Testing Strategy

Reading NMEA sentences from I2C.

TODO or Help Wanted

Documentation Updated

• Updated the relevant files in /docs, or no updates are required.

Formatting

• Ran make prepush.

[lschuermann]

Just a review of the HIL for now. I'm wondering whether we should, at some point, introduce a higher-level HIL that can actually represent position data meaningfully, but a low-level NMEA-Sentence HIL seems fine to me too. It should have its semantics documented more clearly though (i.e. overall end-to-end behavior, and edge cases like timeouts & errors).

[brghena]

This looks reasonable to me. The design looks good enough that the bottom could be swapped out for a UART implementation (which is common) in the future without issue.

Way back in the Signpost days, we also just exposed NMEA messages to userspace, because it's way easier to parse them there. There are various C libraries that will do this for you.

[bradjc]

I would be supportive of a HIL that provides location, but I'm not sure about the benefit of a HIL for NMEA strings. I think that trait can just be in capsules.

[alistair23]

I would be supportive of a HIL that provides location, but I'm not sure about the benefit of a HIL for NMEA strings. I think that trait can just be in capsules.

NMEA can be exposed over UART, I2C or BLE (probably other things as well) so I think it makes sense to use a HIL. That way in the future we can expand the implementations

Comments addressed

[alevy]

I agree with @bradjc about a HIL, particulary one in the sensors (fwiw, NMEA is used not only for sensors, but also actuators, like autopilots). It doesn't need to be a HIL for there to be a trait that's implemented for different serial interfaces. In particular, I'd prefer to have that trait be in capsules (e.g. the thing that is a client of the NMEA can define the trait), and I'd prefer for that to come up when there is at least a second implementation

[alevy]

Are all uses of NMEA over I2C or all uses over UART or ditto over SPI, etc always the same framing interface? E.g. is an I2C implementation guaranteed to always work for all NMEA exposed over I2C? Or does each GPS/NMEA peripheral actually have its own specific I2C protocol?

[alistair23]

Are all uses of NMEA over I2C or all uses over UART or ditto over SPI, etc always the same framing interface?

Yes. The format of a NMEA sentence is defined. It doesn't matter what device or transport method you use

[alistair23]

I moved the trait from a HIL to a capsule

[alevy]

Yes. The format of a NMEA sentence is defined. It doesn't matter what device or transport method you use

@alistair23 sure, but what I meant was are NMEA sentences always accessible over I2C in the same way? For example, it's pretty common for I2C peripherals to have a particular "register" you read different "sensor" data from. In this implementation, it looks like the expectation is that the device will just spit out NMEA over I2C with no other framing, but is that always the case? E.g. this seems to leave no room for configuring the device (GPS peripherals often have a way to switch them to lower/higher power modes, communicate saved position fixes, etc).

[alistair23]

@alistair23 sure, but what I meant was are NMEA sentences always accessible over I2C in the same way? For example, it's pretty common for I2C peripherals to have a particular "register" you read different "sensor" data from. In this implementation, it looks like the expectation is that the device will just spit out NMEA over I2C with no other framing, but is that always the case? E.g. this seems to leave no room for configuring the device (GPS peripherals often have a way to switch them to lower/higher power modes, communicate saved position fixes, etc).

From what I have seen, most devices expose NMEA sentences in the same way. The only thing that changes is the address of the device.

The one big difference is the u-blox DDC mechanism. It allows reading/writing configuration registers via I2C. BUT if you read from the DDC Current Address Read Access (reading with no offset) at the default on startup you will just read the data stream. So even though u-blox have a more advanced configuration, it seems to still be true by default (according to their documentation).

Even if there is a device that does something a different way, it can still use this trait and just modify the operations to access the NMEA data in the device specific way.

You could configure devices by performing I2C write operations, the same way you could configure the device over serial by writing to the UART. That wouldn't go through this interface though, it would be a device dependent configuration.

[alistair23]

Ping!

[bradjc]

I'm still nervous about adding the nmea_i2c driver that has ambiguous expectations, and has all the drawbacks of UART with the overhead of I2C. I think if we are going to have this it should be designed to support receiving every message, and not just by reading 1 byte at a time. I think it should also document which chip this is known to support.

Right now this feels like a very specific tool for a very niche use case, which there is nothing wrong with, but with upstreaming it I don't want to push the rough edges on to the next user.

[alevy]

Yeah, as is, this doesn't seem on path to "right." I'll try to articulate the totality of why I think that and maybe we can either understand if I'm mistaken, or else what the right path to "right" should be.

First, I think broadly NMEA in general, and GPS in particular, is totally in scope for support. There are three ways I think this could play out well, not necessarily mutually exclusively:

1. We have things like a "location" driver that, e.g., updates userland when there is a new location reading, or responds with the most recent location. This would not be NMEA specific, though in many cases (but not all) the underlying implementation would be talking to a GPS peripheral over some NMEA protocol. The use case here is obviously applications that want location. There are of course other things that are communicated over NMEA, and those could/would be separate drivers, so there may need to be some demultiplexing driver on platforms that want multiple such drivers (see 2)

2. An internal NMEA subsystem, in a few different components, that is responsible for abstracting various ways of getting NMEA data (over various kinds of communication layers---ethernet, i2c, uart, BLE, etc---and with all kinds of framing etc protocols), as well as for demultiplexing the data itself for various interested parties (it's output is complete sentences, possible for particular kinds of data, since many devices will broadcast over the same NMEA substrate). The clients of this subsystem could be, for example, an implementation of (1), or other internal components that use NMEA data in one way or another.

3. A minimal userspace driver that allows processes to subscribe to all (or a subset of) NMEA sentences on one or more interfaces. This would also rely on at least some of (2), but not necessary all of it.

I think this PR is closest to trying to provide 3., but misses the mark.

First, the interface from the process's perspective is "please give me, and only me, the next sentence from NMEA immediately." This may fail if someone else is concurrently reading, and will also deprive other potential readers of that sentence. This isn't the right interface.

It's possible that the particular GPS chip that @alistair23 is using does just dump out the same sentence continuously forever, but that seems an extremely uncommon mode of operation and I'm skeptical that even @alistair23's chip doesn't have a mechanism to be notified when new data is available. A subscribe that emits to everyone (subscribed) when there is new data, and/or a command that always returns the most recently read data (unless none has ever been read), would be more reasonable.

Second, as @bradjc has been pointing out, the lower-level NMEA driver only seems to make sense in this very particular, and kind of odd chip interface. Again, if there is some very general i2c handshake protocol that many/all chips that provide NMEA over i2c use, that makes sense maybe as a general i2c_nmea driver, but I'm very skeptical it looks like this, and the trait's semantics don't really match the kind of way you'd imagine using NMEA (well) in practice.

[bradjc]

First, the interface from the process's perspective is "please give me, and only me, the next sentence from NMEA immediately." This may fail if someone else is concurrently reading, and will also deprive other potential readers of that sentence. This isn't the right interface.

The syscall driver is virtualized to support multiple apps.

            // Read sentence
            1 => self
                .apps
                .enter(processid, |app, _| {
                    app.operation = Operation::Read;

                    self.buffer.take().map_or(CommandReturn::success(), |buf| {
                        match self.driver.read_sentence(buf) {
                            Ok(()) => CommandReturn::success(),
                            Err((e, buffer)) => {
                                self.buffer.replace(buffer);
                                app.operation = Operation::None;
                                CommandReturn::failure(e)
                            }
                        }
                    })
                })
                .unwrap_or_else(|err| CommandReturn::failure(err.into())),

If a sentence has already been requested by a different app the driver just marks the app as a reader. When the sentence is ready it is copied to all apps.

    fn callback(&self, buffer: &'static mut [u8], len: usize, status: Result<(), ErrorCode>) {
        for cntr in self.apps.iter() {
            cntr.enter(|app, kernel_data| {
                if app.operation == Operation::Read {
                   ...
                        let _ = kernel_data.get_readwrite_processbuffer(0).and_then(|dest| {
                            dest.mut_enter(|dest| {
                                let copy_len = dest.len().min(len);
                                dest[0..copy_len].copy_from_slice(&buffer[0..copy_len]);
                            })
                        });

                        app.operation = Operation::None;
                        kernel_data.schedule_upcall(0, (into_statuscode(Ok(())), len, 0)).ok();
                    }
                
            });
        }

        ...
    }

[alevy]

Ok @bradjc amended my comment, I read that code wrong.

[alevy]

I think, then, the current status is that both @bradjc and I are skeptical about this nmea_i2c driver. Having a specific chip reference would help.

If that is resolved, I think there is room for, particularly, better documentation (specifically of the system call interface), but might be good otherwise.

[alistair23]

I find this a little hard to follow what exactly you are asking.

The GNSS device is some unnamed MediaTek device. It's possibly https://cdn.sparkfun.com/assets/parts/1/2/2/8/0/GTOP_NMEA_over_I2C_Application_Note.pdf but I'm not sure exactly. It has an antenna stuck on top of the SoC so it's hard to tell.

Second, as @bradjc has been pointing out, the lower-level NMEA driver only seems to make sense in this very particular, and kind of odd chip interface. Again, if there is some very general i2c handshake protocol that many/all chips that provide NMEA over i2c use, that makes sense maybe as a general i2c_nmea driver, but I'm very skeptical it looks like this, and the trait's semantics don't really match the kind of way you'd imagine using NMEA (well) in practice.

I'm not sure I follow, especially the odd chip interface.

From my understanding Brad's proposal is to create a ~300 byte buffer to store the sentences in and read them continuously. The goal being that we never loose any part of the buffer.

As I pointed out in #3881 (comment), that results in a pretty large buffer. It doesn't really guarantee you don't loose anything if userspace isn't quick enough. It also falls apart with low power mode.

I can rename nmea_i2c to als_nmea_i2c if that helps?

The other option might be for someone else to grab a GNSS device, hook it up and test if this works for them. I've had this one running for hours and don't see any issues with missing sentences. Userspace is easily able to decode the data and obtain a location

[bradjc]

I've had this one running for hours and don't see any issues with missing sentences.

How are you checking if messages are getting dropped? From my understanding, the only way this could receive every NMEA message is if all NMEA messages from this chip are a multiple of I2C_BUFFER_LEN. Is that the case? Otherwise the $ and all subsequent characters from the "second" message will be dropped, and the entire message is lost.

From my understanding Brad's proposal is to create a ~300 byte buffer to store the sentences in and read them continuously. The goal being that we never loose any part of the buffer.

As I pointed out in #3881 (comment), that results in a pretty large buffer. It doesn't really guarantee you don't loose anything if userspace isn't quick enough. It also falls apart with low power mode.

I'm not sure that is that large of a buffer, but, in any case, that may just be the cost of writing a robust driver. No one would use NMEA if they were aggressively trying to reduce memory as any ASCII protocol is going to have significant overhead.

Have you measured the power? This seems like not the way to write a low power driver. I would expect an interrupt driven interface rather than a polling one like this.

I'm also worried about clock stretching. If this driver issues a 24 byte read, but the chip only has 1 byte ready, the chip has to clock-stretch until the next 23 bytes are available. If that takes 1 second, the bus is consumed for that entire interval, and nothing else can use I2C.

[alistair23]

How are you checking if messages are getting dropped? From my understanding, the only way this could receive every NMEA message is if all NMEA messages from this chip are a multiple of I2C_BUFFER_LEN. Is that the case? Otherwise the $ and all subsequent characters from the "second" message will be dropped, and the entire message is lost.

You would also get every message if you read fast enough that the next message isn't in the buffer yet.

I more meant that I receive all of the NMEA types, not that I guarantee that I get every message.

I'm not sure that is that large of a buffer, but, in any case, that may just be the cost of writing a robust driver. No one would use NMEA if they were aggressively trying to reduce memory as any ASCII protocol is going to have significant overhead.

It is a large buffer and all of these buffers add up. You also need a buffer in userspace to copy it to. This ends up wasting memory, which Tock systems are generally limited on.

AFAIK MediaTek GNSS devices only support NMEA. So unless you are using u-blox, your custom silicon or maybe other vendors you are going to use NMEA. Even for other devices NMEA is a go to. It's easy to decode and there are a bunch of existing libraries for it.

Have you measured the power? This seems like not the way to write a low power driver. I would expect an interrupt driven interface rather than a polling one like this.

Yes, I have measured the power. The GNSS when running uses a large amount of power (~26mA compared to ~8mA from Tock). I can set the GNSS device to lower power, which basically puts the GNSS to sleep and it wakes up at some schedule, depending on the low power mode.

Then from userspace I can just attempt a read. If the GNSS device is asleep it Nacks the transaction (really it just doesn't Ack it) so userspace gets an error. Then userspace can sleep and try again.

I can also wait for an interrupt from the GNSS device via GPIO and then start the read after that. That requires connecting an interrupt pin though, which is a bit trickier to setup.

At least my device also supports GPIO wakeups, but I haven't tried that.

I'm also worried about clock stretching. If this driver issues a 24 byte read, but the chip only has 1 byte ready, the chip has to clock-stretch until the next 23 bytes are available. If that takes 1 second, the bus is consumed for that entire interval, and nothing else can use I2C.

I haven't found a single GNSS device that clock stretches. Let me know if you know of any that do.

Everything I see just Nacks when you read the FIFO.

[bradjc]

I haven't found a single GNSS device that clock stretches. Let me know if you know of any that do.

Everything I see just Nacks when you read the FIFO.

Oh! This is helpful to know! This workflow makes a lot more sense, but isn't in the code or the PR description. I didn't know how this driver is working.

[alistair23]

I always thought clock stretching was very uncommon. Although it's allowed it is rarely used/supported.

I can add a comment to the code saying we expect the device to Nack when the FIFO is empty if that helps?

[alistair23]

Comment added

[alistair23]

Ping!

[alistair23]

Ping!

[alistair23]

Also, the waiting-on-author tag doesn't really work. As it doesn't get removed when no longer waiting on the author

[alistair23]

Ping!