I think it makes sense :-) There are some things I am not quite sure I get…
So I think the pointer approach like you describe below is the better way, with the possible exception of gpio (I will explain that later). Having a hal_spi_dev * or hal_adc_dev * passed to the hal api is a common way to do it and I like it better than the sysid approach as you dont have the extra overhead for each call. The possible exception to this is gpio and is related to the part I dont get, which is ram/code usage. Consider a simple MCU with 3 spi ports and 64 gpio. For the SPI, the BSP would have three hal_spi_dev structures (devid 0, 1, and 2) defined. These are in .text and each take 8 bytes (stores a pointer and the device id). We need 4 bytes of RAM per SPI. We also need the driver structure (in flash). We only need one of these and it is a set of N function pointers (N * 4 bytes in .text). Now consider the gpio. There would be 64 of these 8 byte data structures (in. text)? And we would need (worst-case), 64*4 bytes in RAM? And also N*4 bytes of .text for each gpio API? I do realize you dont have to define all 64... Folks probably wont like this idea; heck, I am not even sure I do… but I think in most cases a simple gpio interface suffices. GPIO are logically ordered (i.e. they have a sysid) and the code in hal_gpio.c in hw/mcu knows how to map that logical gpio for the MCU. The BSP maps the sysid to something the application can use. Basically, what we have today for gpio. Maybe I just should not worry about the ram/code usage for gpio (but I do) :-) > On Mar 23, 2016, at 7:58 PM, [email protected] wrote: > > I was thinking the same as you. I had one thought for a way to get this > functionality. Im actually pretty excited about it. Hope this makes sense. > > We could declare a structure in hal_xxx.h > > struct hal_xxx_device; > > This could be the ³this" argument to all the hal_xxx functions. > > Example > > int hal_adc_read( struct hal_adc_device *padc); > > > In the hal_xxx_int.h we can define this like this. > > Struct hal_xxx_driver Š (same as we have now).. > > Struct hal_xxx_device { > Struct hal_xxx_driver *pdrv; > uint8_t devid; /* really only need 8 bits but > structures PAD */ > uint8_t reserved[3]; > } > > And then define the shims as something like in hal_adc_int.c: > > inline int hal_adc_read(struct hal_xxx_device *pd) { > If(pd && pd->pdrv && pd->pdrv->adc_read) { > return pd->pdrv->adc_read(pd->pdrv, pd->devid); > } > Return -1; > } > > > Whether these are inline or not I don¹t care as long as the are NOT > MACROS. > > Now suppose I am writing a library. The library needs some hal devices to > do battery charging control. > > Typedef struct my_battery_library_state { > Extern const struct hal_*gpio_device pon; // to set the > LED status of > the battery > Extern const struct hal_*adc_device ptemp; // to read the > battery > temperature > Extern const struct hal_*adc_device pvoltage; // to read the > battery > voltage > Extern const struct hal_*pwm_device pcurrent; // to set the > output > current level > } > > The library would have to get these values from the BSP via function call. > Undefined variables won¹t consistently work in case the library author > intends there to be multiple battery chargers. > > int bsp_get_power_library_led_gpio(int charger_instance, Struct > hal_gpio_device **out); > > The bsp could typically implement as a const structure (since in most > cases this is probably fixed by the board design). > > > > Const struct hal_adc_device temp_adc_for_battery_monitor = { > > &samd21_adc_controller, /* this type of driver */ > 0, /* device ID 0 */ > } > > int bsp_get_power_library_led_gpio(int instance, const struct > hal_gpio_device **out) { > /* This BSP only knows how to provision one battery monitor. */ > If(instance != 0) { > Return -1; > } > *out = & temp_adc_for_battery_monitor; > Return 0; > } > > > > > Analysis: > > For the sake of analysis, I¹ll consider two things using the HAL: > 1) A library which doesn¹t know at compile time which hal_device it will > use. > 2) An application which does know at compile time. > > * the structure definition could be totally hidden from the application > writer with the hal_shim. They just need to keep a pointer. > * This structure API is probably faster than the sysid API since we don¹t > have the BSP dispatch on every API call. > * This would use 8 bytes of code space for each device (const) used and > simple function to fetch the device from the library. > > * This uses way less code space as we don¹t have all the shim code for > every hal. The hal shim could be a simple inline like above. > * This would dispense with the sysid concept which might make the whole > BSP mapping simpler. There¹s really no need to map all devices to > Some arbitrary sysid just so we can map them back to a device. > * An application could just extern the const structures and use > &my_hal_device in the code which would not take RAM. With the sysid > approach these > Would be constants and also take no RAM. > * A library would have to store space for all the pointers which is 4 > bytes * number of devices. The sysid model would depend on the size of > the sysid. > Could be as small as 1 byte, so the sysid approach is BETTER for RAM > usage in a library. Imagine 20 GPIOs. Its either 20*1 = 20 bytes of RAM > (sysid) or > 20 * 4 = 80 bytes of RAM. > > The RAM usage concerns me a bit, but I think the idea of removing the > sysid seems like it would make things overall simpler for the user. > > Comments? > > > > > > > > > On 3/23/16, 4:59 PM, "will sanfilippo" <[email protected]> wrote: > >> I think the hardest part for me to ³get over² (if you will) is the fact >> that hal_xxx_init() does not return a pointer to something and that each >> of the API has to call the BSP function every time. However, I do >> understand why you would want to do it the way you did (at least I think >> I understand). >> >> I do wish that some of the API were a bit more abbreviated but that is >> only because i dont like typing :-) >> >> And btw, I am interested in hearing the answer to marko¹s questionŠ >> >> Will >> >>> On Mar 23, 2016, at 4:03 PM, marko kiiskila <[email protected]> wrote: >>> >>> Good stuff. >>> >>>> On Mar 22, 2016, at 5:21 PM, [email protected] wrote: >>>> >>>> All, >>>> >>>> I'm having so much fun with my newt. Please comment and help me >>>> improve this work. >>>> >>>> I've submitted two HAL API pull requests. They are to add new >>>> HAL_xxx.h files for two new sets of core functionality: ADC and PWM. >>>> >>>> When designing these hal_xxx.h interfaces, I considered the APIs from >>>> mbed-hal and from arduino-hal. I treated these as "enough" with a few >>>> caveats. >>>> >>>> 1. Generally, APIs that set specific state of devices seems hard to >>>> maintain and the system designer will have to know about them anyway. >>>> So I made Apis in the ADC hal to query the resolution and reference >>>> voltage rather than set them. They will be set by the MCU unless >>>> configurable, then they can be set by the BSP >>>> 2. There were duplicate ways in mbed to set PWM duty cycle. Rather >>>> than implement them all, I implemented a sufficient subset. Future >>>> versions could expand on this, or someone could write a helper library >>>> on top of it to covert between the various methods. >>>> >>>> https://github.com/apache/incubator-mynewt-core/pull/22 - this is a >>>> pull requests for the hal api for the Pulse Width Modulation. . >>>> https://github.com/apache/incubator-mynewt-core/pull/21 - this is the >>>> pull request for the hal API for the Analog to Digital Converters. >>>> >>>> Underlying HAL philosophy. I tried to following this philosophy when >>>> doing the interface. >>>> >>>> 1. A given device may support multiple channels (I.e. One ADC >>>> controller can sample 8 ports). Its needs a single driver since its one >>>> device multiplexed. >>>> 2. A number of different PWM/ADC devices can be supported at the same >>>> time. >>>> 3. Its possible and likely that there will be N instances of the same >>>> Device driver with just different state (e.g. Two ADC devices with 8 >>>> channels each that are identical except for memory map). >>> >>> That¹s good. >>> >>>> So I implemented the HAL as follows: >>>> >>>> 1. for each hal_xxx.h there is a set of sysid (system_ids). These >>>> represent individual xxx (e.g. ADC) resources in the system. The >>>> adc_sysid and pwm_sydid are different number spaces. >>>> 2. The hal_xxx apis all take a sysid to reference the device and port. >>>> 3. There is an underlying hal_xxx.c implementation in the hal that >>>> resolves the sysid into a device_id and a driver interface. This is >>>> not the implementation of the hal, just a shim to call the BSP to get >>>> the driver and devid before calling back into the driver itself. >>>> 4. There is an underlying driver that implements the driver interface >>>> that can support up to N device Ids. >>>> >>>> As a concrete example, imagine a system with two ADC devices. >>>> 1. A SPI ADC with 10-bits precision and 8 pins >>>> 2. An internal MCU ADC with 8 bits precision 16 pins >>>> >>>> The BSP will be in charge of mapping a system ID into the device >>>> drivers and Ids. For example: >>>> >>>> 1. sysID 0-7 belong to SPIADC 0-7 >>>> 2. sysID 8-24 belong to MCUADC 0-15 >>>> >>>> Each individual driver will manage their devices and not need to know >>>> how they map to system Ids. >>> >>> How do you map from devid to particulars of HW configuration? For >>> example, how would tell that for my Olimex board, I want >>> to use pin X as input for my ADC? Or that my PWM for sysid 0 should go >>> out to pin Y? >> >
