Add generic serial bus driver #1507
Conversation
andrzej-kaczmarek
force-pushed
the
bus-dev
branch
2 times, most recently
from
78ed30c
to
75318d9
Compare
| assert(measurement); | ||
|
|
||
| rc = bme280_set_sensor_mode(BME280_FORCED_MODE, &bme280_node->bme280_dev); | ||
| assert(rc == 0); |
There was a problem hiding this comment.
asserting on access may be to drastic. Maybe rc could be returned on errors.
In release build when assert is not present function returns 0 event if data is wrong.
| static void | ||
| close_node_cb(struct bus_node *node) | ||
| { | ||
| console_printf("%s: node %p\n", __func__, node); |
There was a problem hiding this comment.
I guess dependency to console is needed
There was a problem hiding this comment.
there's already dependency in bus_test app and since this is sub-package not meant to be used outside I think this is enough
| #endif | ||
|
|
||
| struct lis2dh_node_pos { | ||
| uint16_t x; |
There was a problem hiding this comment.
I think it maybe signed data
There was a problem hiding this comment.
yeah, I just used this a raw data since it has no other purpose than demo ;-)
andrzej-kaczmarek
force-pushed
the
bus-dev
branch
from
75318d9
to
7b7202e
Compare
|
Changed:
|
|
|
||
| /* XXX this is only required for SPI, but apparently device has no problem | ||
| * with this being set also for I2C so let's leave it for now since there's | ||
| * no API now to figure out bus type for node |
There was a problem hiding this comment.
For devices who need to know this, they should be able to keep track of it themselves. Maybe no need for such an API.
There was a problem hiding this comment.
that's a good point, I did not think about it
hw/bus/src/bus.c
Outdated
|
|
||
| err = os_mutex_release(&bdev->lock); | ||
| if (err == OS_BAD_MUTEX) { | ||
| return SYS_EACCES; |
There was a problem hiding this comment.
I suspect people will ignore return codes from this routine, so I would assert on OS_BAD_MUTEX. Probably should allow OS_NOT_STARTED.
There was a problem hiding this comment.
agree, will fix
hw/bus/src/bus.c
Outdated
| return SYS_ETIMEOUT; | ||
| } | ||
|
|
||
| assert(err == OS_OK); |
There was a problem hiding this comment.
Probably should allow OS_NOT_STARTED.
hw/bus/src/bus.c
Outdated
|
|
||
| assert((void *)bdev == (void *)bnode->parent_bus); | ||
|
|
||
| if (bdev->configured_for == bnode) { |
There was a problem hiding this comment.
Good. No need to configure if it already is.
hw/bus/src/bus.c
Outdated
| return rc; | ||
| } | ||
|
|
||
| rc = bus_dev_configure_for(bdev, bnode); |
There was a problem hiding this comment.
I'm thinking you'll want to combine bus_dev_lock_by_node() with bus_dev_configure_for(). The latter is always preceded by latter. Would be less calls.
There was a problem hiding this comment.
agree, configuring bus for device while locking it sounds reasonable.
hw/bus/src/bus.c
Outdated
| return 0; | ||
| } | ||
|
|
||
| static inline int |
There was a problem hiding this comment.
I would recommend dropping inline. I think we care more about space rather than speed.
There was a problem hiding this comment.
good point
| #include "bus/bus_driver.h" | ||
|
|
||
| static int | ||
| bus_node_open_func(struct os_dev *odev, uint32_t wait, void *arg) |
There was a problem hiding this comment.
Arguments wait nor arg are not used. Do you intend these to be used at some point?
There was a problem hiding this comment.
probably not (I don't really see how I could use those parameters here), but this is callback from os_dev so it needs to be like this.
| .configure = bus_spi_configure, | ||
| .read = bus_spi_read, | ||
| .write = bus_spi_write, | ||
| .disable = bus_spi_disable, |
There was a problem hiding this comment.
For SPI devices combined read/write is occasionally needed. I see there's an call API matching combined read/write semantic in bus_node(), but no such thing here?
I think there's 2 options here; either add combined read/write to allow that for SPI, or make the bus_node_XXX interface such that the driver writer can take advantage of the locking/reconfig for bus-sharing done there, and then they can call hal_XXX routines of their choice while the device is locked/configured.
Including the non-blocking read/write version. Or were you going to roll those as separate interface within bus_dev_ops()?
There was a problem hiding this comment.
bus_node_write_read_transact does write-then-read to simplify perhaps one of most common transaction for peripheral devices. I also had write-and-read API prototype but removed it as I did not implement it since I don't really know device which would require it (I've only seen ones that can use it, but not require). This can be added later as a bus_node API and as a new op here (like write_read). My original idea was that this would work as expected for SPI bus and return SYS_ENOTSUP for I2C bus. For now, as you mentioned, one can just lock bus manually via bus_dev_lock_by_node (btw, I don't like this name, thinking of changing it) and use HAL if really necessary.
Once I add async APIs which can use non-blocking HAL calls I think it will require to add non-blocking versions of read, write (and read-write) ops as well.
There was a problem hiding this comment.
Given that it is a public API, I'm all for changing it. bus_node_lock()/bus_node_unlock() ?
I suspect it's ok to leave the full-duplex operation out of the bus_node interface.
|
Fixed latest comments. Also had to update device creation since when adjusting other drivers it turned out that some of them need custom arg to be passed to init function - the one we have for Finally, BME280 sample node is removed since it did not pass RAT check as it uses driver from Bosch. If current API is good enough I'd prefer to merge this now and fix/update with subsequent PRs - I guess before release we can even adjust some APIs if necessary since they are not frozen. |
andrzej-kaczmarek
changed the title
hw/bus/include/bus/bus.h
Outdated
| } | ||
|
|
||
| /** | ||
| * Read data from node |
hw/bus/include/bus/bus.h
Outdated
| * This is simple version of bus_node_write() with default timeout and no flags. | ||
| * | ||
| * @param node Node device object | ||
| * @param buf Buffer to read data into |
hw/bus/include/bus/bus.h
Outdated
| } | ||
|
|
||
| /** | ||
| * Read data from node |
hw/bus/include/bus/bus_driver.h
Outdated
| int (* enable)(struct bus_dev *bus); | ||
| /* Configure bus for node */ | ||
| int (* configure)(struct bus_dev *bus, struct bus_node *node); | ||
| /* Write data to node */ |
hw/bus/include/bus/bus_driver.h
Outdated
| /* Write data to node */ | ||
| int (* read)(struct bus_dev *dev, struct bus_node *node, uint8_t *buf, | ||
| uint16_t length, os_time_t timeout, uint16_t flags); | ||
| /* Read data from node */ |
| rc = bus_node_simple_write(dev, payload, len); | ||
|
|
||
| done: | ||
| rc = bus_dev_unlock_by_node(dev); |
This adds initial implementation of generic serial bus driver. General bus driver is an abstraction for using serial buses like I2C or SPI in a generic way where bus state is managed automatically to handle access form multiple threads and to multiple devices concurrently in a seamless way. This aims to replace direct calls to HAL in drivers etc. Internally bus driver works with two main object types: - bus_dev = bus device, a representation of I2C or SPI bus instance - bus_node = bus node, a representation of device connected to bus_dev In general, bus_dev object creation should be managed by MCU (or BSP) code and replace existing calls HAL init APIs since it will initialize these HALs internally. bus_node objects should be created by drivers or by an application in case application wants to access some "loose" device on the bus without dedicated driver. Externally both objects are passed around as simple os_dev so basically application needs to call os_dev_open() to get os_dev object and then just pass it to bus APIs.
This adds init function which gets generic structure as a parameter as opposed to hal_spi_init which requires MCU-specific structure. This is to make it compatible with latest similar addition to I2C HAL.
With bus driver enabled sensor interface does not need to manage locks and device configuration so let's just remove this part in such case. This means we needs all sensors used by sensor interface to be either bus-driver-compatible or legacy-compatible depending on what we use.
This modified driver to be compatible with bus driver, when enabled. As a bonus the driver supports now both I2C and SPI when used with bus driver. Helper functions are added to register bus node for both I2C and SPI (only one shall be registered for single device).
This is a simple app for evaluating hw/bus driver.
GPIOs are int everywhere.
To create os_dev which is a bus node for sensor device we need to pass both node configuration and sensor interface. To make this easier, there's now sensor_node_cfg struc provided which contains both data and can be passed when creating os_dev.
Since bus nodes are os_dev, they may need some extra argument to be passed in init function. However, os_dev custom argument is already used by bus driver thus we need other way to pass driver-specific arg. To make this possible, bus_XXX_node_create functions have extra argument which is stored inside node and passed to init function later. Hack warning: to avoid using extra 4 bytes of memory for each node to store extra arg for a brief time we'll just reuse the same pointer (via union) as used by parent_bus. It's ok to make it like this since we need to store arg until init function is called and also parent_bus is used 1st time in init function so we can just "exchange" contents there.
Bus locking APIs names were changed to more friendly bus_node_lock() and bus_node_unlock(). Also locking bus will automatically configure bus for given node since we lock the bus in order to talk to given node so can have extra call here. bus_node_unlock() will not return an error since it's a bit useless - probably no one cares about it and it can only fail if things are really broken already, so we'll just assert internally to aid debugging.
BME280 sample node used driver from Bosch which I'm not sure if we can include in Mynewt repository. Let's remove it - we have sample node for LIS2DH written from scratch and BME280 is supported via sensors interface.
andrzej-kaczmarek
force-pushed
the
bus-dev
branch
from
d038876
to
072aa6b
Compare
|
@rymanluk fixed I have some more fixes ready for this but would prefer to put them in separate PR as it becomes more and more time consuming to rebase all of those patches :-) |
This PR adds initial proposed API and implementation of generic serial bus driver.
The goal is to create abstraction for using serial buses like I2C and SPI in a generic way where bus state is managed automatically to allow access from multiple tasks and to multiple devices concurrently in an easy way [1]. It will be also good place to add special handling for known issues in existing devices if they are attached to bus so we can recover from errors [2].
[1] currently we use global mutexes which are shared via sensor/charger/etc. interface with each driver and each driver needs to lock it manually - we should have lock defined and managed automatically on bus level
[2] currently we already have some workaround for faulty device in
hal_i2cimplementation ofhw/mcu/nordic/nrf52xxx- HAL is not a good place to workaround problems not related to particular I2C controller and this workaround will also not work when platform other thannrf52xxxis usedThere are two kind of objects managed by bus driver:
bus_dev= bus device which is a representation of I2C or SPI bus instance; should be created by MCU (or BSP) code and effectively replace existing init calls to HAL since this is done now internally by bus driver,bus_node= bus node which is a representation of an actual device connected to bus; should be created by driver or even an application in case application wants to access some "loose" object on the bus without dedicated driver.It may be a bit counter-intuitive to use
bus_devfor bus and not for an actual device, but it originated fromos_devwhich is used internally and honestly I don't have any better names for both so any other ideas are welcome. I eventually got used to these names after a week of coding ;-)The setup to use bus driver for I2C controller is as follows:
i2c0bus_devwhich initializes hardware (similar to whathal_xxx_initcall did previously)bus_nodewhich specifies:bus_dev(by name) where node is attachedApplication which wants to talk to one of created
bus_nodes should do as follows:os_dev_open()to openbus_nodewith specified nameos_devobtained fromos_dev_open()call can now used to talk to device usingbus_nodeAPIsEach operation on
bus_nodedoes as follows:bus_devis locked for exclusive accessbus_devis unlockedSo effectively what users of
bus_nodes need to do is just call singlebus_node_xxxAPI and parse the result. They don't need to care about current bus state. The goal is to be able to use a simple call to make most of bus accesses.APIs are split into two main parts:
bus/bus.his what is enough to accessbus_nodeas opened byos_dev_open()and this is what driver and application will mostly usebus/bus_driver.his what is necessary to createbus_devandbus_nodefrom MCU/BSP/driver so more low-level stuff that needs to be only done once to createos_devs and not required to access bus from apps(there's also
bus/bus_debug.hbut it's just some helper for debug code, so not really useful from "outside")Current API allows to do following operations:
bus_nodebus_nodebus_node(with bus lock held during entire transaction) which is probably the most commonly used transactionbus_devto allow combine the above simple ops into compound transactionsbus_devlock1/2/3 are available as blocking operations; non-blocking versions of these APIs will be also available and they will quite similar to blocking calls except the result will be returned via supplied callback
4 I considered to provide API to define and execute compound transactions (other than most common write->read) but abandoned this for now since it will be quite complicated and I am not sure if it will be used too much, so probably not worth the effort, but we can still add this later
5 is to provide some compatibility with existing code so we can share mutex with other existing ways of locking, at least for now
In addition to new
hw/busandhw/bus/i2cpackages which implements generic bus driver and I2C bus driver, there are few other commits included which were either necessary or just useful to implement this:hal_i2cis extended with some calls that already exist inhal_spiwhich are separate calls to enable, disable and configure interfacehw/mcu/nordic/nrf52xxxis extended to createbus_devs for enabled I2C instances whenhw/busis included in buildapps/bus_testis provided which includes two simple implementation of bus_nodes which it can then talk to (they are running at different speeds just to see if things are reconfigured properly on-the-fly)Finally, there are few things that are not finished yet which I'll try to update in upcoming days:
UPDATE 2018-11-08: