Hey, thanks for this mail. it's great that we're working through this stuff! when responding to mail, people should feel to cull aggressively, as this thread is getting very long. However, I wanted to include everything for context on the first response. :-)
On 3/1/16 10:04 PM, [email protected] wrote: > This was a thread between Will and Marko and I but wanted to share it with > the list for feedback . Sorry its a commented mess, but wanted to get it out > there for other to see. > > The topic was about how to clean up an organize the hal abstraction. > Specifically, should this be broken up into smaller bits. I think we are > converging. Next step would be to document this and then get the existing > HAL into alignment. > > > I talked offline with Will and Marko about this topic at length today. > We > have a proposed resolution of how this would work. Of course its an > evolution, but if we are all thinking similarly this will go smoother. > > The hal (hardware abstraction layer which is synonymous with hardware > interface) will be broken up based on functionality (e.g. UART, SPI, > ADC, > etc). In the descriptions below we used hal_adc as an example since it > feels like it has sufficiently complicated functionality. > > Each component hal will contain a the header file (e.g. hal_adc.h) and > small bit of code (e.g hal_adc.c). The header file will provide the > > I am not quite sure I understand the “small bit of code” comment. Is this > code that resides in hw/hal/adc/src or is this code small because, in > effect, it is calling function pointers provided by a structure from the > bsp? I guess I am a bit confused on where hal_adc.c is located. Will it > now always be located in hw/hal/adc or can hal_adc.c be in the mcu or > drivers directory? > > PFD ― I was thinking that there would always be a small part in > hw/hal/hal_adc.c which fetches the instance from the BSP and then calls > through to function pointers just like in the flash code. > > programming interface for users. Here’s a simple example ADC hal api. > > int hal_adc_init(int adc_device_number); > int hal_adc_polling_read(int adc_device_number, int adc_channel); > Multiple levels of indentation and threading are in the above, making it really hard to understand. Can somebody summarize this? > The hal_adc.c driver would be responsible for mapping the device number > to > the right driver implementation and calling the underlying driver > functionality. This may be as simple as calling driver->polling_read(int > adc_channel) or we could layer in functionality to provide some higher > level services. At the time hal_adc_init is called, the hal_adc.c code > would call the BSP to get the driver for that particular interface. For > example. > > const hal_adc_driver_interface *bsp_provide_hal_adc_driver(int > adc_device_number); > > The BSP would be responsible for mapping the virtual device number to a > physical interface. For example a BSP may have to implement the > following. > > const hal_adc_driver_interface *bsp_provide_hal_adc_driver(int > adc_device_number) { > Switch(adc_device_number) { > Case 0: > return &ads4149_driver_interface; > Case 1: > return &samd21_adc_driver_interface; > Default: > return NULL; > } > Just to clarify, is this to handle both internal & external ADCs? Otherwise, I would think the ADC driver implementations would live in the hw/mcu definition. If the desire is to map both internal and external ADC drivers, then I can see this being useful, as the HAL ADC driver can come from either the MCU definition or a driver. > > Then the hal_adc.c would call the underlying driver init. The > underlying > driver init may (but not always) need to call the BSP to get pin info or > other stuff on the part (if its necessary). These functions are often > (likely) device specific. For example, > > const samd21_adc_info *bsp_provide_samd21_adc_info(int > adc_device_number, > int adc_channel); > > Some driver implementations may not need to call the BSP at all since > there is nothing to configure that is BSP specific. > > What does this all look like in the package files….. > > A user who is using a hardware device will include the hal definition in > their yml file. This user may not be the system implementer but also > could be the implementer of a library like a battery monitoring library > or > a temperature logging library using some external parts. For example, > > # the battery monitoring library monitors battery temperature and > voltage > and provides a call back interface. Its requires ADC support with 2 > # ADC channels, one for monitoring voltage and one for monitoring > temperature. > pkg.name: libs/battery_monitor > pkg.vers: 1.0.0 > pkg.deps: hw/hal/adc > > The hal interface (e.g. hal_adc.c) will have a required_capability > (req_cap) on the the system providing the underlying driver. For > example. > > pkg.name: hw/hal/adc > pkg.vers: 1.0.0 > pkg.req_caps: hw/drivers/adc > Is this how we name capabilities? I thoug > > ht this would be something like > adc_driver as opposed to hw/drivers/adc > > PFD ― I assumed it was, but that was a total guess. I was thinking that > we would want a namespace here (and not just a name) since there could be > other virtual interfaces as well. I like the idea of hal/XXX and > drivers/XXX so its clear that the driver is for the hal interface. > I agree with this. I really see a difference between the driver and MCU implementation, which is that a driver should ideally cross multiple MCU definitions. > Drivers could appear in two places. First they could be part of an mcu. > For example, processor X in hw/mcu/processorX could have this in their > package file. > > pkg.name: hw/mcu/processorX > pkg.vers: 1.0.0 > pkg.caps: hw/drivers/adc > > Drivers could also appear in the hw/drivers directory. For example in > hw/drivers/ADS4149 (a SPI based ADC part) there could be an ADC driver > implementation for this TI part that has a SPI interface. It would > depend > on hal/spi (since it requires a spi interface) and provide drivers/adc > > pkg.name: drivers/ads4149 > pkg.vers: 1.0.0 > pkg.deps: hw/hal/spi > pkg.caps: hw/drivers/adc > > What would newt do…. > > Newt would resolve missing components before compiling. It could > generate > some error or warning messages like. > > Unresolved interface: Package hw/drivers/ads4149 depends on hw/hal/spi > which requires a hw/drivers/spi implementation. Use newt search > pkg.caps=hw/drivers/spi to find possible implementations. > > Or > > Unresolved interface: Package hw/hal/adc requires a hw/drivers/adc > implementation. Use newt search pkg.caps=hw/drivers/adc to find > possible > implementations > This is how it currently works. The error messages are not quite that nice, but the system should refuse to compile if no matching capability is found. What's missing now is I don't think we handle multiple packages requiring a specific capability properly. So, for example, if: - hw/mcu/stm32f4xx/ and - hw/drivers/external_adc Both satisfied the hw-hal-adc capability, I don't believe that both of them would be in the include path of the package that required the hw-hal-adc capability. We should fix that, and probably have a directive to include at least 'n' of a certain capability. > A few notes: > 1) the pkg.caps and pkg.req_caps seems a bit confusing. Maybe we should > use pkg.requires and pkt.provides for this concept. > Sounds great. I agree. We should make this change prior to v0.8-b2. > Typo. You meant pkg.caps and pkg.provides. Personally I dont mind the > capabilities terminology but I am also fine with the requires/provides > terminology. Latter probably better but not sure worth the change; just > dont know. > PFD -yes. I will clarify > > 2) We have not sorted out how drivers interface with pin muxing or > interrupts. This could either be through another pkg.deps: hal/irq or > through the call to bps_provide_xx_adc_info(int adc_device) from the > driver to the HAL. The former would take some thought but really make > the > users job easier in the end. > 3) newt could make the warnings above. I think its helpful because the > user may get a better idea of what is going on and how to provide what > is > needed. However, newt can’t actually know if the user is calling > hal_adc_init() which would in tern call the bsp code from the hal to > provide the interface. So it may be overly restrictive to stop people > in > newt. Would we want these to be errors, warning? I think this is up to the BSP to decide. If the BSP requires the HAL ADC to be present, then we should enforce this. I think we should break the HAL up into a set of more fine-grained APIs personally. There should be hal-api-core, which is what we need to run the operating system. hal-api, which is the full HAL. And then hal-api-adc, hal-api-pwm, etc. That should give users flexibility on what they include. Cheers, Sterling
