> On Dec 18, 2017, at 5:25 PM, Jacob Rosenthal <[email protected]> wrote: > >> >> 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
I think if you are using one of the sensors that Mynewt has support for and you would like to contribute back support to suspend and resume the sensor, it would be great to have that in the code base as an example. Also, if you have written some drivers and they do not support suspend and resume functionality, I think it’s totally fine. Certain drivers have certain features and it’s totally normal to have support for one feature and not have support for the other. > * Sensors (at least the ones we have implemented) don't need the open/close > handlers and thats fine. Yes, that is correct. > * 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? It is not an oversight, it was a decision to not open them as I did not want them to be accessed using the os_dev/sensor framework and I was only using the shell to experiment with those. > * We should finish implementing os_dev's suspend/resume functionality and > implement it for the sensors As I mentioned earlier, having an example in the codebase is nice but not necessary. If you would love to contribute back support for suspending/resuming some sensor, that would be cool :-) > > > 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. Sounds good. Regards, Vipul Rahane > > > 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 >> >> >> >>
