> > The name is what os_dev provides along with os_init callback that is used > by other devices. It might/might not be useful dependent on what device you > want to use it on. >
Id push back the idea that subsystems should be built and consume memory before an implementation need arises. But thats a separate thread probably. Ill try to repeat what you said to make sure I understand: * The reason sensors should be os_dev is so they can implement suspend/resume so we should do that * Sensors (at least the ones we have implemented) don't need the open/close handlers and thats fine. * But it was an oversight for those 5 sensors to not do an os_dev open presumably? since.. if you're not going to claim the resource it seems like you're misusing it? * We should finish implementing os_dev's suspend/resume functionality and implement it for the sensors Also, we were talking about sensor_itf on Saturday. Do you think you have a > plan in mind for that ? I do not see it being mentioned here. If you would > love to contribute towards that, I have some implementation in mind: Absolutely, but wanted to keep this single issue first. On Mon, Dec 18, 2017 at 6:11 PM, Vipul Rahane <[email protected]> wrote: > Hello, > > Answers inlined. These are my thoughts based on some discussion we had in > house and earlier a few months ago. > > > On Dec 18, 2017, at 2:58 PM, Jacob Rosenthal <[email protected]> > wrote: > > > > Im trying to wrap my head around os devices and sensors and am finding > the > > current implementations odd. > > > > From what I can gather, os_dev_create creates an os device which mainly > > provides the benefit of being able to open,suspend, resume,close a > > resource. It does this by calling a callback (device init) function after > > it finishes whom it expects to register the handler functions > > > > But oddly NOTHING in mynewt utilizes suspend or resume functionality, and > > only adc, uart and pwm even register open/close handlers. > > Suspend and resume handlers should be used along with > hal_bsp_power_state() which takes state as an input which would be either > application specific or BSP specific. Looking at the state one could > abstract when the os_dev_suspend() and os_dev_resume() get called fro ma > state machine. These handlers would be implemented in the driver. Drivers > can be very specific if needed or can be really generic. It all depends on > what one needs in their product. Having said that, polling is the most > basic configuration of a sensor that people start with along with various > configuration options. It helps them in starting off and contribute to the > OS in a meaningful manner. If some drivers do not contain deep > sleep/suspend/resume functionality, contributors can help in adding such > functionality to them. > > Note: I think we do not have OS_DEV_SETHANDLERS_PWR_STATE() or a similar > macro to set handlers for suspend and resume which we would have to add. > > > Even more odd is that none of the slew of sensors registers ANY handlers > > with os_device, and are therefore completely wasting the memory > associated. > > Mynewt has a lot of optional stuff which might or might not be needed by > all or part of hardware or BSP. > > > > > Proof of this lies in the fact that only one out of the six upstream > sensor > > implementations (the shell implementations) even bothers to find and open > > its existing device > > https://github.com/apache/mynewt-core/blob/master/hw/ > drivers/sensors/bma253/src/bma253_shell.c <https://github.com/apache/ > mynewt-core/blob/master/hw/drivers/sensors/bma253/src/bma253_shell.c> > > This basically slipped through. It seems like a good use of os_dev, > though. But, I would not want the sensor I am experimenting with to get > suspended or resumed, I want raw access to the sensor to play with it. > > > > > The other five leave the os device closed and they make a fresh > interface. > > https://github.com/apache/mynewt-core/blob/master/hw/ > drivers/sensors/bmp280/src/bmp280_shell.c#L39 > > https://github.com/apache/mynewt-core/blob/master/hw/ > drivers/sensors/bno055/src/bno055_shell.c#L45 > > https://github.com/apache/mynewt-core/blob/master/hw/ > drivers/sensors/bno055/src/bno055_shell.c#L45 > > https://github.com/apache/mynewt-core/blob/master/hw/ > drivers/sensors/tcs34725/src/tcs34725_shell.c#L38 > > https://github.com/apache/mynewt-core/blob/master/hw/ > drivers/sensors/tsl2561/src/tsl2561_shell.c#L55 < > https://github.com/apache/mynewt-core/blob/master/hw/ > drivers/sensors/tsl2561/src/tsl2561_shell.c#L55> > > Reason for using a fresh interface is: > > The sensors come up with a fresh configuration. Shell is solely used for > experimentation purposes/manufacturing. Configuration of a sensor happens > in the BSP or in the sensor creator. This is the default configuration. > When someone is experimenting with a sensor, they start off using the clean > factory default config. Once the config is specified in the shell and all > the sensors are experimented with, the developer would lock in certain > configurations that they would want and do a re-configuration of the > sensors at various points in the state machine. > > The name is what os_dev provides along with os_init callback that is used > by other devices. It might/might not be useful dependent on what device you > want to use it on. > > > > > Which as I said, actually makes sense since opening doesnt do anything > > > > Which isnt to tear down any work by anyone involved, Its not like I > offered > > any input when this was all being merged originally :) > > > > Thoughts on what the usefulness of os_dev is and should be and thus how > > sensors should or shouldn't be built on top of them? > > Also, we were talking about sensor_itf on Saturday. Do you think you have > a plan in mind for that ? I do not see it being mentioned here. If you > would love to contribute towards that, I have some implementation in mind: > > For the macro you were mentioning: > > ODI_GET_ITF() macro which takes the os device as an argument. > > SENSOR_GET_ITF() macro which takes the sensor as an argument would have to > be changed to return os_dev->od_itf. > > For the os_dev_itf structure: > > os_dev_itf should be a separate structure and there should be a pointer to > it in os_dev. > > For compatibility with public APIs using sensor_itf, in sensor.h we could > do this: > > #define sensor_itf os_dev_itf > > Hope this clears up most of your doubts if not all. Others are welcome to > chime in as well. os_devs existed before sensors did so, please feel free > to correct me. > > Regards, > Vipul Rahane > > > >
