u-blox / ubxlib

Portable C libraries which provide APIs to build applications with u-blox products and services. Delivered as add-on to existing microcontroller and RTOS SDKs.
Apache License 2.0
313 stars 98 forks source link

Zephyr devicetree API #93

Closed mtfurlan closed 10 months ago

mtfurlan commented 1 year ago

ubxlib is not integrated with the devicetree model, this is confusing and limiting. Devicetree wants you to to have a device(on a bus) and load the driver based on the compatible string, while ubxlib wants you to setup the bus in the devicetree, put no device on it, and use preprocessor defines to select what bus to use.

Aside from being confusing, this makes supporting two boards that have different u-blox chips on different busses very difficult. For other things like status lights, I just configure the devicetree so that the LED named "gnss_fix" is on an IO expander on board A, on IO on board B, and the codebase doesn't need to know how it's connected. But for ubxlib, I have to additionally set some preprocessor flags to select the ublox bus and chip type in addition to telling zephyr which board devicetree to use.


The devicetree configuration stuff is available at compile time, so I suspect that a zephyr driver could handle converting the devicetree configs into the defines ubxlib is expecting. There would definitely be issues if someone tries to have two ublox modules connected, and probably some other edge cases because drivers are supposed to be self contained and pass around a pointer to a struct for all of the configuration (example)

I'll try to look into this more, but I'm doing this for a $work project that I'm being moved off of, so I can't promise much.

mtfurlan commented 1 year ago

I made a very first pass dts shim that seems to work for my usecase.

dts/bindings/sensor/ublox,ubxlib-shim.yaml:

description: |
  A shim to put ubxlib in the devicetree

compatible: "ublox,ubxlib-shim"

include: base.yaml

properties:
  bus-type:
    type: int
    enum:
      - 0
      - 1
    required: true
    description: type of bus it's on, 0=spi, 1=i2c
  bus-num:
    type: int
    required: true
    description: what number bus it's on (4 for spi4, for example)
  module-type:
    type: int
    enum:
      - 0
      - 1
      - 2
    required: true
    description: What type of ublox module, M8=0, M9=1, M10=2

devicetree overlay:

&spi4 {
    compatible = "nordic,nrf-spim";
    status = "okay";
    cs-gpios = <&gpio0 17 GPIO_ACTIVE_LOW>;
    pinctrl-0 = <&spi4_default>;
    pinctrl-1 = <&spi4_sleep>;
    pinctrl-names = "default", "sleep";
    ublox: ublox@0 {
        compatible = "ublox,ubxlib-shim";
        reg = <0>;
        bus-num = <4>;
        bus-type = <0>; // 0 is spi, 1 is i2c
        module-type = <1>; // 0 is M8, 1 is M9, 2 is M10
    };
};

main.c:

#define UBLOX DT_NODELABEL(ublox)
#ifdef UBLOX
    #if DT_PROP(UBLOX, bus_type) == 0 // spi
        #pragma message ( "spi" )
        #define U_CFG_APP_GNSS_SPI DT_PROP(UBLOX, bus_num)
    #elif DT_PROP(UBLOX, bus_type) == 1 // i2c
        #pragma message ( "i2c" )
        #define U_CFG_APP_GNSS_I2C DT_PROP(UBLOX, bus_num)
    #else
        #warning "bad ublox dts config"
    #endif
    #define U_CFG_TEST_GNSS_MODULE_TYPE DT_PROP(UBLOX, module_type)
#else
    #warning "no ublox in dts?"
#endif
...
static const uDeviceCfg_t gDeviceCfg = {
    .deviceType = U_DEVICE_TYPE_GNSS,
    .deviceCfg = {
        .cfgGnss = {
            .moduleType = U_CFG_TEST_GNSS_MODULE_TYPE,
            .pinEnablePower = -1,
            .pinDataReady = -1, // Not used
        },
    },
#if defined(U_CFG_APP_GNSS_I2C) &&  (U_CFG_APP_GNSS_I2C >= 0)
    .transportType = U_DEVICE_TRANSPORT_TYPE_I2C,
    .transportCfg = {
        .cfgI2c = {
            .i2c = U_CFG_APP_GNSS_I2C,
            .alreadyOpen = true
        },
    },
#endif
#if defined(U_CFG_APP_GNSS_SPI) &&  (U_CFG_APP_GNSS_SPI >= 0)
    .transportType = U_DEVICE_TRANSPORT_TYPE_SPI,
    .transportCfg = {
        .cfgSpi = {
            .spi = U_CFG_APP_GNSS_SPI,
            .pinMosi = -1, 
            .pinMiso = -1, 
            .pinClk = -1,
            .device = U_COMMON_SPI_CONTROLLER_DEVICE_INDEX_DEFAULTS(0)
        },
    },
#  endif
};

I had spent a while trying to figure out how to get "4" out of the spi4 devicetree defines so you don't have to configure it into the shim, and I came to the conclusion you can't. And without doing string comparison on the "compatible" of the spi/i2c bus you can't figure out if it's spi/i2c, and the preprocessor doesn't want to do string comparisons.

It needs some extending to support things besides i2c and spi, and extended to support other configs like alternate i2c addresses, but I think this is our best approach for the short term.

This meets my needs, so while I'll definitely post if I come up with something better I'm probably just going to use this.

RobMeades commented 1 year ago

That's really interesting: I'm on holiday for a week but will take a look at it when back.

A question: we're adding Linux support right now, should be in for the end of July release, and for that we've had to add a "prefix" API (pasted in below) to accommodate the string needed to get at a given UART device. We will likely have to do a similar thing for I2C (Raspberry Pi being our main use-case). Are there additions like this, i.e. APIs added to ubxlib in a backwards-compatible way, that could make accommodating a device-tree-driven approach more "complete"?

/** Set the uart name prefix to be used in the next call to
 * uPortUartOpen().
 *
 * On some platforms this name will prefix the uart parameter
 * passed to uPortUartOpen(); for example a prefix of "/dev/tty"
 * with a uart value of 3 will result in device "/dev/tty3"
 * being opened as a UART.  If the uart parameter passed to
 * uPortUartOpen() is < 0 then only the prefix will be used
 * (e.g. "/dev/tty").
 *
 * This is currently only applicable for the Linux port.
 * The default prefix if this function is not called will be
 * #U_PORT_UART_PREFIX.
 *
 * @param[in] pPrefix  a pointer to a string containing the name
 *                     prefix, up to #U_PORT_UART_MAX_PREFIX_LENGTH
 *                     characters long.
 * @return             zero on success else negative error code.
 */
int32_t uPortUartPrefix(const char *pPrefix);
mtfurlan commented 1 year ago

Stop checking your email on holiday :)

Right now with my bodge version of a devicetree config method, it would be nice if I could just pass ubxlib the struct* gpio_dt_spec for the bus instead of passing a number, just for ubxlib do get the struct* gpio_dt_spec with GPIO_DT_SPEC_GET_BY_IDX in getSpiCsControl or wherever.

I'm just not sure if that would be a helpful step towards the final goal of being able to configure ubxlib by devicetree and not involving uDeviceCfg_t, or if it would just have to get removed later.

We should first figure out what having an actual devicetree driver looks like and then decide how to get there. My intuition says that doing it all at once would be better then as steps, but I have not yet done a devicetree driver and therefore don't really know.

RobMeades commented 1 year ago

Agreed. I'm going to propose we make this a goal for release 1.4. And I will stop checking my e-mail now :-).

Maybe ;-).

EDIT: we should probably ask Zephyr where lieth the stable API for being driven from the Zephyr device tree.

RobMeades commented 1 year ago

FYI: I have asked in zephyrproject: https://discord.com/channels/720317445772017664/883445484923011172/1128021841076817980.

RobMeades commented 1 year ago

While performing some domestic duties (I am on holiday but am at home), a thought occurred to me: we could introduce a new uDeviceTransportType_t, called something like U_DEVICE_TRANSPORT_TYPE_PRECONFIGURED. The idea would be to support cases where the configuration of the device is done "out of band" by the SDK, accommodating the Zephyr and Linux approaches. The struct to go with it might look something like this:

/** Preconfigured transport, supported only by the likes of Zephyr and Linux.
  */
typedef struct {
    uDeviceVersion_t version;       /**< Version of this structure; allow your
                                         compiler to initialise this to zero
                                         unless otherwise specified below. */
    uDeviceTransportType_t type;    /**< The type of the transport that has been
                                         preconfigured: UART, I2C, SPI etc.; must
                                         NOT be set to
                                         #U_DEVICE_TRANSPORT_TYPE_PRECONFIGURED,
                                         that would be silly.  Where this is an I2C
                                         device, this is equivalent to a
                                         #uDeviceCfgI2c_t with alreadyOpen set to true. */
    const char *pName;              /**< A pointer to the null-terminated name of the
                                         transport, which must have meaning to,
                                         must have been already configured by, the
                                         underlying platform (for example
                                         uart0 or /dev/ttyUSB0); cannot be NULL. */
    /* This is the end of version 0 of this structure:
       should any fields be added to this structure in
       future they must be added AFTER this point and
       instructions must be given against each one
       as to how to set the version field if any of
       the new fields are populated. For example,
       if int32_t pinMagic were added, the comment
       against it might end with the clause "; if this
       field is populated then the version field of
       this structure must be set to 1 or higher". */
} uDeviceCfgPreconfigured_t;

Note: for the SPI case this assumes that the peer device configuration (bit-ordering, clock edge, etc.) can also be implied by the same single pName; if not then we may need to add a const char *pSpiDeviceName and maybe an int32_t spiDeviceIndex; this needs checking.

Then in each of the uPortUart, uPortI2c and uPortSpi APIs we add a uPortXxxOpenPreconfigured(const char *pName) function which can be used instead of the existing uPortXxxOpen() functions (and maybe an uPortSpiControllerSetDevicePreconfigured(const char *pDeviceName, int32_t deviceIndex), pending checking the SPI situation).

The effective change is to pass the name in from the top, from the uDevice API, rather than trying to put the bits of it together later from the int32_t and a prefix etc., the impossible task. This new mechanism could then be supported by Linux and Zephyr, and the big win from it could be that, via this API, the Zephyr port layer might actually be generic, the UART of an Atmel chip would work, 'cos all the naming differences in the device tree are irrelevant to us... I hope? We would still support the "old way" for existing chipsets, but those using Zephyr and Linux get the choice of a much easier to use API that properly supports any chipset.

@mtfurlan: would this help?

bperseghetti commented 1 year ago

@mtfurlan @RobMeades what is the timeline on this we have a similar issue using Zephyr 3.4 but for UART.

RobMeades commented 1 year ago

@bperseghetti: if you are well versed in the device tree, can you comment on whether introducing a uDeviceTransportType_t U_DEVICE_TRANSPORT_TYPE_PRECONFIGURED as described above, would allow you to work in the way you would like, driven by the Zephyr device tree? Adding such an API would not be a big job, I just don't want to do it if it is not going in the right direction for you.

bperseghetti commented 1 year ago

Yeah, let me look to make sure, @beriberikix you also have any thoughts/inputs?

mtfurlan commented 1 year ago

I don't think uDeviceTransportType_t type and const char *pName changes anything, we would still needs to know what bus type and what bus "name" we're on at compile time. Just instead of passing a bus number we would have a string we need to parse before putting it into that switch to find the gpio_dt_spec.

With zephyr I want to pass in a struct *gpio_dt_spec, and we can figure out at runtime what type of device it is. But that isn't what linux needs at all.

Now I'm on holiday, so while I can't actually play around with anything and I don't have dedicated time, I'll keep thinking about this.

beriberikix commented 1 year ago

@bjarki-trackunit might have some good input as well!

RobMeades commented 1 year ago

For backgound, since a few people are hooked into this now, ubxlib has a structure which defines a u-blox device (what kind (cellular, wifi, BLE, GNSS) and the specific type (M8 v M9 v SARA-R5 v NINA-W15 etc.)) and the details of the transport the MCU is using to connect to that device (all of the varied aspects of UART, SPI or I2C).

https://github.com/u-blox/ubxlib/blob/efdc592cb2aa840900e1c6591d00124f41eb8867/common/device/api/u_device.h#L325

The elements of these structures are integers (enumerated types, integer pin numbers, integer HW block numbers) and are passed into the ubxlib port layer at run-time as integers.

This was all fine and good until Zephyr landed with the string-based device tree.

mtfurlan has very sensibly suggested that ubxlib might look at a way to accommodate Zephyr users with a specific API that allows not only the transport stuff to be taken directly from the device tree but also the ubxlib-specific device stuff (module kind and type) to be brought in in the same way, a way that is natural for Zephyr users. I'd kinda hoped that the same approach might also apply for Linux, which we are just adding support for, given the device tree commonality, but maybe that's a hope too far.

Anyway, that's the aim: invent a "device-tree sympathetic" addition to the transport portions, and maybe the GPIO portion, of the ubxlib port layer API, somehow tunneled through the ubxlib uDevice layer API.

bjarki-andreasen commented 1 year ago

Hi, regarding the devicetree, bus attached devices and gpio descriptions are supported by the devicetree without adding any new property or definitions.

Bus attached devices

I2C and SPI attached devices are "natively" supported. They import the bindings from i2c-device.yaml and spi-device.yaml. For an example from the devicetree of a device placed on a SPI bus, see b_l4s5i_iot01a dts(lines 144 to 169). For an example of determining and adjusting the the used bus type, see bmi323 driver(note macros like DT_INST_ON_BUS(inst, spi))

ubxlib and Zephyr device drivers

Zephyr does not support dynamically loading device drivers and modules. When Zephyr builds a board, it is known exactly what actual hardware is connected to which bus. The devicetree "compatible" property contains the exact hardware module placed on the board (M8, M9, M10), device drivers can support multiple hardware modules and revisions. Each specific modem has an associated bindings file, which defines exactly what hardware (pins and properties etc) are available to that specific modem.

Example of ublox modem dts

&spi3 {
    pinctrl-0 = <&spi3_sck_pc10 &spi3_miso_pc11 &spi3_mosi_pc12>;
    pinctrl-names = "default";
    status = "okay";

    cs-gpios = <&gpiod 13 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>,

    gnss@0 {
        compatible = "ublox,neo-m9n";
        reg = <0>;
        power-enable-gpios = <&gpioa 8 (GPIO_ACTIVE_LOW | GPIO_PULL_UP)>;
        spi-max-frequency = <2000000>;
    };
};

Example of ublox modem bindings file

Only add exactly the pins and busses this particular hardware revision supports: (note: I don't know which pins this module supports, its just an example)

compatible: "ublox,neo-m9n"

include: ["spi-device.yaml", "i2c-device.yaml"]

properties:
  power-enable-gpios:
    type: phandle-array
    description: |
      Power enable gpio

Example device driver

This shows the end of a device driver supporting multiple hardware revisions.

...
...
...

#define UBLOX_NEO_M9N_DEVICE(inst)
...

#define UBLOX_NEO_M10_DEVICE(inst)
...

#define DT_DRV_COMPAT ublox_neo_m9n
DT_INST_FOREACH_STATUS_OKAY(UBLOX_NEO_M9N_DEVICE)
#undef DT_DRV_COMPAT

#define DT_DRV_COMPAT ublox_neo_m10
DT_INST_FOREACH_STATUS_OKAY(UBLOX_NEO_M10_DEVICE)
#undef DT_DRV_COMPAT

To summarize:

No changes to devicetree and macros are needed.

Lastly, to expose ubxlib features to the user, there is no generic API in Zephyr, create bespoke APIs for ubxlib in include/drivers/modem/<your_modem_specific_header_here.hpp> to expose driver specific features. There are already examples in that folder to use.

RobMeades commented 1 year ago

Thanks for the comprehensive input @bjarki-trackunit: we will think on't and allow mtfurlan to comment when he returns from holidays.

mtfurlan commented 1 year ago

The steps @bjarki-trackunit outlined look like the way to implement this actually correctly.

I only posted my stopgap solution above so we have something until we get a full and proper devicetree implementation. Also I don't know much about the device driver side or the ubxlib interface side.

The next step is to make a device driver and see what new interfaces/interface changes we need in ubxlib. I got moved to a different project at work so if someone else can take over that would be nice, otherwise I'll try to carve out an hour or two a week for this.

RobMeades commented 1 year ago

@mtfurlan: understood. The members of the ubxlib team who know more about the Zephyr device tree are from our Swedish office, which has entered the Swedish holiday season now, so not a great deal is going to happen on this front for at least the next three weeks.

Your suggestions, coming at it from a Zephyr angle, would be greatly valued, if you are able to find the time in the interim.

bperseghetti commented 1 year ago

How much of this issue has overlap with https://github.com/u-blox/ubxlib/commit/336e6acc90fed8b9824f85198cecdcf9311eba69 and other more recent commits that I have not had the time yet to dig into?

RobMeades commented 1 year ago

@bperseghett: https://github.com/u-blox/ubxlib/commit/336e6acc90fed8b9824f85198cecdcf9311eba69 is a step in the right direction, in that it resolves the problem of Zephyr platforms that don't follow the naming convention that ubxlib expects in the device tree for UART/I2C/SPI.

What we have not yet done is added support for custom tags that would allow ubxlib-specifics, such as the module type, to be passed in directly from the device tree.

RobMeades commented 11 months ago

@bjarki-trackunit: I'm getting to grips with the DT macros etc. to implement this but am getting an error with the reg property; can you point me at what I might do to fix/debug this?

Though I don't need it I understand that the property is required so I have used 0 for it as you suggested above:

&uart0 {
    compatible = "nordic,nrf-uarte";
    status = "okay";
    current-speed = <115200>;
    pinctrl-0 = <&uart0_default>;
    pinctrl-1 = <&uart0_sleep>;
    pinctrl-names = "default", "sleep";
    // My bit with @0 and reg = <0>
    cfg-device-cellular@0 {
        compatible = "u-blox,ubxlib-device-cellular";
        reg = <0>;
    };
};

...however the Device Tree parser throws this error:

devicetree error: 'reg' property in <Node /soc/uart@40002000/cfg-device-cellular@0 in '/nrfconnectsdk-v2.5.0/zephyr/misc/empty_file.c'> has length 4, which is not evenly divisible by 12 (= 4*(<#address-cells> (= 2) + <#size-cells> (= 1))). Note that #*-cells properties come either from the parent node or from the controller (in the case of 'interrupts').

I've thrown a few different patterns of the reg property at it but have been unable to make it happy so far.

bjarki-andreasen commented 11 months ago

sure, child devices of uarts don't have addresses (the @0 and regs =<0>) unlike I2C and SPI bus devices

RobMeades commented 11 months ago

Oh! OK, now I know I suppose. Thanks for the remarkably swift response.

RobMeades commented 11 months ago

@mtfurlan, @bperseghetti, @bjarki-trackunit: as my Christmas project I have been trying to, finally, get this done.

I have something which works but there is a "however": for nodes such as &uart0, &spi0, &i2c0 and also for gpios phandles, there is no way, through Zephyr DT macros, to obtain the peripheral type and the HW block number, e.g. to get the uart and the 0, or to get the x from gpiox and the pin, or even to get "uart0" as a string that can be parsed at run-time.

These two things are fundamental to the way the porting layer of ubxlib works.

Hence I've had to do things the other way up, i.e. to have ubxlib device nodes which, within them, have ubxlib-style configuration properties for the peripherals, following the pattern of the ubxlib device configuration structures for UART, SPI and I2C and using plain-old pin numbers. I've included an example .dts file below containing all possible DTS properties (the one I am testing with), where the nodes that use i2c and spi show this most clearly. To be clear, only the pin numbers of any peripheral used by ubxlib is dictated by the &blah peripheral node, every other property is dictated by the ubxlibnode: for instance, if you don't specify spi-frequency-hertz in a ubxlib node that uses SPI you get the ubxlib SPI default frequency and not one you might have specified in your &spix node; that is ignored.

This is not ideal: it means that some of the "nativeness" of this approach is lost. I could use the generic property names from the xxx-controller.yaml files for UART, SPI and I2C in my bindings, and I could make transport a sub-node to line it up better with the ubxlib structure it maps to, but I can't see any way of allowing the .dts file to have just transport = &uart0 and pwr-on-gpios = <gpio0 10 0>, which would be most native, 'cos the peripheral type and HW block numbers aren't exposed, or can't other than hackily be inferred, from/through the DT macros.

Your thoughts on this would be appreciated.

    cfg-device-cellular-0 {
        compatible = "u-blox,ubxlib-device-cellular";
        status = "okay";
        transport = "uart0";
        baud-rate = <57600>;
        module-type = "U_CELL_MODULE_TYPE_SARA_R422";
        pin-enable-power = <0>;
        pin-pwr-on = <10>;
        pin-vint = <35>;
        pin-dtr-power-saving = <36>;
        network = <&label_cfg_network_cellular_thingy &label_cfg_network_gnss_inside>;
    };
    cfg-device-cellular-1 {
        compatible = "u-blox,ubxlib-device-cellular";
        status = "okay";
        transport = "uart3";
        network = <&label_cfg_network_cellular_thingy &label_cfg_network_gnss_inside>;
    };
    cfg-device-cellular-2 {
        compatible = "u-blox,ubxlib-device-cellular";
        status = "okay";
        transport = "uart2";
    };
    cfg-device-gnss-0 {
        compatible = "u-blox,ubxlib-device-gnss";
        status = "okay";
        transport = "i2c0";
        i2c-clock-hertz = <1000>;
        i2c-already-open;
        i2c-address = <43>;
        module-type = "U_GNSS_MODULE_TYPE_M9";
        pin-enable-power = <1>;
        pin-data-ready = <36>;
    };
    cfg-device-gnss-1 {
        compatible = "u-blox,ubxlib-device-gnss";
        status = "okay";
        transport = "spi2";
        spi-frequency-hertz = <2000000>;
        spi-index-select = <0>;
        spi-mode = <2>;
        spi-word-size-bytes = <3>;
        spi-lsb-first;
        spi-start-offset-nanoseconds = <5>;
        spi-stop-offset-nanoseconds = <10>;
        module-type = "U_GNSS_MODULE_TYPE_M8";
        pin-enable-power = <2>;
        pin-data-ready = <37>;
    };
    cfg-device-gnss-2 {
        compatible = "u-blox,ubxlib-device-gnss";
        status = "okay";
        transport = "uart4";
        baud-rate = <230400>;
        gnss-uart2;
    };
    cfg-device-short-range-0 {
        compatible = "u-blox,ubxlib-device-short-range";
        status = "okay";
        transport = "uart1";
        baud-rate = <115299>;
        module-type = "U_SHORT_RANGE_MODULE_TYPE_NINA_W15";
        open-cpu;
        network = <&label_cfg_network_ble_peripheral &label_cfg_network_wifi_client_home>;
    };
    cfg-device-short-range-1 {
        compatible = "u-blox,ubxlib-device-short-range";
        status = "okay";
        transport = "uart2";
        network = <&label_cfg_network_ble_peripheral &label_cfg_network_wifi_client_home>;
    };
    cfg-device-short-range-2 {
        compatible = "u-blox,ubxlib-device-short-range";
        status = "okay";
        transport = "uart2";
    };
    label_cfg_network_cellular_thingy: cfg-network-cellular-thingy {
        compatible = "u-blox,ubxlib-network-cellular";
        apn = "blah";
        timeout = <30>;
        username = "fred";
        password = "blogs";
        authentication-mode = <1>;
    };
    label_cfg_network_gnss_inside: cfg-network-gnss-inside {
        compatible = "u-blox,ubxlib-network-gnss";
        module-type = "U_GNSS_MODULE_TYPE_M10";
        device-pin-pwr = <9>;
        device-pin-data-ready = <32>;
    };
    label_cfg_network_wifi_client_home: cfg_network_wifi_client_home {
        compatible = "u-blox,ubxlib-network-wifi";
        ssid = "my_home_ssid";
        authentication = <2>;
        pass-phrase = "my_pass_phrase";
        host-name = "my_host_name";
        mode = "STA_AP";
        ap-ssid = "my_home_ap_ssid";
        ap-authentication = <6>;
        ap-pass-phrase = "my_ap_pass_phrase";
        ap-ip-address = "1.1.1.100";
    };
    label_cfg_network_ble_peripheral: cfg-network-ble-peripheral {
        compatible = "u-blox,ubxlib-network-ble";
        role = "PERIPHERAL";
        enable-sps-server;
    };

The main bindings files are also attached, renamed to .txt.

u-blox,ubxlib-device-cellular.txt u-blox,ubxlib-device-gnss.txt u-blox,ubxlib-device-short-range.txt

RobMeades commented 11 months ago

I have now tidied this up and pushed a preview branch here with a better explanation of how it is intended to be used.

bjarki-andreasen commented 11 months ago

I'm sorry, but this is not compatible with the Zephyr project. The device driver model is not just a convenience, its one of the most essential pillars the project rests upon.

Drivers added to the project must adhere to the device driver model. That is, implement and adhere to the existing devicetree bindings and device driver APIs.

The following is an example of how a modem is presented in the devicetree:

/* This is the uart node,  transport = "uart2"; will not be accepted */
&usart1 {
    current-speed = <115200>;
    hw-flow-control;
    pinctrl-0 = <&usart1_default>;
    pinctrl-names = "default";
    status = "okay";

    modem: modem {
        power-domain = <&pd_modem>;
        compatible = "quectel,bg95";
               /* This is a GPIO pin,  device-pin-data-ready = <32>; will not be accepted */
        mdm-power-gpios = <&piob 14 GPIO_ACTIVE_LOW>;
        zephyr,pm-device-runtime-auto;
        status = "okay";
    };
};

This is what the library must adhere to. Adding properties like transport = "uart2"; and device-pin-data-ready = <32>; will not be accepted into the project :(

I would suggest you research the following:

This may bring some light to the issue :)

RobMeades commented 11 months ago

Hi @bjarki-trackunit and thanks for the comment on Christmas eve eve!

The problem is that, unfortunately I can't see any way to adopt what you suggest. Not only have my researches of the last week not turned up any way to do it but mbolivar, who I guess is an authority on this from his presentation at the Zephyr development conference on macrobatics (I think that was him?), said:

not right now, no. the token blah is not the node's name, it is a node label. you cannot get the node labels for a node right now.

...in answer to my query:

Given a node:

&blah { ... };

...and something which refers to that node:

thingy = &blah;

...is there a DT macro that I can apply to thingy which will get me the name blah? In other words, not all of the stuff that blah refers to, with registers and the like, just its name?

Of course, I've no particular need for any of this to be accepted into Zephyr, that is not the intention, the intention is just to provide a "Zephyr sympathetic" mechanism for Zephyr users to provide the configuration that ubxlib needs. It would be nice to do it the canonical way: that just doesn't seem to be possible.

bjarki-andreasen commented 11 months ago

The primary issue seems to be the ubloxlib including its own HAL, defining its own uart drivers and hardware layout language.

Trying to repurpose the devicetree to match a HAL that is not founded in the devicetree is not feasible. The devicetree includes many rules that are well defined in the devicetree specification v0.4.

You will likely find adapting ubloxlib to use the devicetree and Zephyr device driver model to be far more practical, compared to trying to adapt the devicetree and device driver model to fit the library.

RobMeades commented 11 months ago

You are probably right but that would result in ubxlib having a platform-specific porting layer, which is the antithesis of a porting layer. And it would only be because, for a given phandle, we are unable to determine what type of peripheral it is pointing to (UART. I2C, SPI or GPIO) and the HW block number of that peripheral, which don't seem like odd things to ask (FYI I've asked about this again, both in Discord and as a Github issue, in case I've missed something).

Anyway, thanks for your guidance. It is Christmas Eve so I will let this rest for a while now and see if others comment on the usefulness or not of what is possible.

Have a very good Christmas.

mtfurlan commented 11 months ago

While it would definitely be better if ubxlib could be 100% compatible with devicetree, it looks like what @RobMeades proposed is still more useful than nothing. At least with that you can define two different board definitions that setup a ubxlib driver for different hardware.

I'll try out that preview branch in the next few days.

RobMeades commented 11 months ago

@mtfurlan: thanks, your feedback would be appreciated.

mtfurlan commented 11 months ago

Thoughts so far:

I'll keep poking and rule out wiring issues by using other code.

RobMeades commented 11 months ago

Yes, it all gets insanely complicated very quickly. Is it best to link to the .overlay file we use for testing:

https://github.com/u-blox/ubxlib/blob/preview_zephyr_dt_rmea/port/platform/zephyr/runner/boards/ubx_evkninab3_nrf52840.overlay

...or to the bindings files:

https://github.com/u-blox/ubxlib/tree/preview_zephyr_dt_rmea/port/platform/zephyr/dts/bindings

EDIT: actually, I'd assumed that a "native" Zephyr user would use some form of GUI to create the overlay, which would display the descriptions in the bindings files: is that what you did?

RobMeades commented 11 months ago

It is worth noting that my testing so far has been at a "logical" level, i.e. I have a test which checks that the values from the .overlay/.dts file (the one linked-to above) ends up in the uDeviceCfg_t etc. structures, I haven't actually done an end-to-end test that checks this has the desired effect on the target HW so there could still be a bug there.

EDIT: if you have ubxlib debug prints on (should be the default) then, when the Zephyr board configuration is picked up, you should see a debug print something like this:

U_PORT_BOARD_CFG: using GNSS device "cfg-device-gnss-1" from the device tree, module-type 2 with pin-enable-power 2, pin-data-ready 37...
U_PORT_BOARD_CFG: ...GNSS on SPI 2, spi-pin-select -1, spi-index-select 0, spi-frequency-hertz 2000000, spi-mode 2, spi-word-size-bytes 3, spi-lsb-first, spi-start-offset-nanoseconds 5, spi-stop-offset-nanoseconds 10, [sample delay 0 nanoseconds, fill word 0xffffffff].
RobMeades commented 10 months ago

@mtfurlan: I'm just testing an end-to-end case myself, I will let you know when I'm sure the full implementation works.

RobMeades commented 10 months ago

@mtfurlan: I've tested that this works end-to-end for cellular, haven't been able to do GNSS/SPI as I couldn't find a spare GNSS/SPI board but the calls are all the same from the core code and the unit test of picking up all types of configuration items passes so it should work. The only issue I fixed (new commit pushed) was that the uNetworkInterfaceUp() part of it wouldn't work properly but I don't think you're using that, just uDeviceOpen().

EDIT: found a GNSS board with SPI and I believe that it is obeying the stuff I have put in the .overlay file. Attached find the .overlay file I have used for this testing (nrf5340dk_nrf5340_cpuapp.overlay, M10 GNSS device on spi2, MOSI pin 1.26 MISO pin 1.25 CLK pin 1.27 CS pin 1.14 (i.e. the pin at CS index 1)) and here's the logged output from the code when run:

U_APP: Running exampleGnssPos...
U_PORT_BOARD_CFG: using GNSS device "cfg-device-gnss-0" from the device tree, module-type 2 with pin-enable-power -1, pin-data-ready -1...
U_PORT_BOARD_CFG: ...GNSS on SPI 2, spi-pin-select -1, spi-index-select 1, spi-frequency-hertz 1000000, spi-mode 0, spi-word-size-bytes 1, spi-start-offset-nanoseconds 0, spi-stop-offset-nanoseconds 0, [sample delay 0 nanoseconds, fill word 0xffffffff].
U_GNSS: initialising with ENABLE_POWER pin not connected, transport type SPI.
U_GNSS: sent command b5 62 0a 06 00 00 10 3a.
U_GNSS: decoded UBX response 0x0a 0x06: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a7 19 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 a8 0b 0a 00 00 00 00 00 [body 120 byte(s)].
...
RobMeades commented 10 months ago

We will be making our 1.4 release in a few week's time; it doesn't seem likely that Zephyr will provide the DT macros we require to adopt the conventional transport-type = &uart0/pwr-on-gpios = <gpio0 10 0> notation, rather than the string/integer-based transport-type = "uart0"/pin-pwr-on = <10> notation, in that time; my inclination is to go ahead with what is possible, since it is better than nothing.

All opinions/improvements still welcomed.

mtfurlan commented 10 months ago

I was able to get a modified examples/gnss/pos_main.c to run with the new interface, I passed NULL to uDeviceOpen instead of a config. I agree this is way better than the current state of affairs.

RobMeades commented 10 months ago

@mtfurlan: glad you got it working, your analysis is much appreciated; we will go with this, deficient as it is.

RobMeades commented 10 months ago

This is now pushed to master here, see commit https://github.com/u-blox/ubxlib/commit/d86f0c6223909e30a7ec48f87083f6da104d5132: sorry it took so long and isn't quite what you wanted. I will delete the preview branch and close this issue.