Skip to content

Units (humidity, distance) #869

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
krwq opened this issue Nov 27, 2019 · 33 comments
Closed

Units (humidity, distance) #869

krwq opened this issue Nov 27, 2019 · 33 comments
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation

Comments

@krwq
Copy link
Member

krwq commented Nov 27, 2019

Since we have experimentally added Temperature I haven't seen anyone complaining about this and I also personally find it very convenient. Since then we have added also Pressure and it also seems convenient.

I think we should make a step forward and add couple more units to avoid (or reduce consequences) breaking changes in the future:

  • distance
  • humidity (RH, with perhaps some utils for absolute etc)
  • (not sure) 3 units for IMU - on one hand Vector3 is fine but on the other hand this could've been more convenient (at the same time this will probably have many open ends)
  • voltage
  • current
  • power

@shaggygi @Frankenslag @ZhangGaoxing @MarkCiliaVincenti @Ellerbach @joperezr @pgrawehr

@krwq krwq added the api-suggestion Early API idea and discussion, it is NOT ready for implementation label Nov 27, 2019
@krwq krwq changed the title Units Units (humidity, distance) Nov 27, 2019
@pgrawehr
Copy link
Contributor

Since we have also PR #679 which reads NMEA sequences, what about speed, position and angles / direction?
Another unit that is commonly used with environmental sensors attached to a Pi is PPM (parts per million) for concentration of gases or air quality.

@krwq
Copy link
Member Author

krwq commented Nov 27, 2019

I'm not sure about derivatives or multidimensional values yet but angles I think is a good idea

@Ellerbach
Copy link
Member

  • distance

Yes, distance would be a great one for sure. To make easy the international to non international units also take correctly in consideration the multiple elements for the international one (cm, m, km, etc).

  • humidity (RH, with perhaps some utils for absolute etc)

Yes as well, for both relative, absolute ans specific (see https://en.wikipedia.org/wiki/Humidity).

  • (not sure) 3 units for IMU - on one hand Vector3 is fine but on the other hand this could've been more convenient (at the same time this will probably have many open ends)

I guess issue is more about using the same units than the Vector3 (and 4) which are perfect for this. So units for IMU can be great as well to help having coherence in the units as well as easy way to convert them.
That will help for angles (degree vs radians) as well as acceleration in G or milli G.

speed, position and angles

Like for distances, speed is a good one as not everyone is using the international speed system (m/s) and even those using express them in km/h for example for cars. So I'll vote for it.

For position, @pgrawehr can you give me an example? Is it terrestral position with logitude and latitude? Or something else? In case of terrestral one, I would vote for yes as well as there are multiple ways to express them as well

PPM

If this is standard and every one is using it with no convertion, then not sure it's needed.

@krwq
Copy link
Member Author

krwq commented Nov 27, 2019

@Ellerbach the only thing for units as Position/Velocity/Acceleration is that velocity can be anywhere from 1 to 3 dimensional depending on context which I'm not sure how would that be abstracted so being conservative I think we should give it more thought (unless everyone is happy with Position, Position2, Position3, Velocity, Velocity2, Velocity3, Acceleration, Acceleration2, Acceleration3)

Speed is not a vector so can be added (or perhaps should we treat it as 1 dimensional velocity)

@shaggygi
Copy link
Contributor

Referencing....

https://www.ccontrols.com/pdf/BACnet_Units.pdf

https://project-haystack.org/doc/Units

@pgrawehr
Copy link
Contributor

Looking at shaggygi's list: General question is whether we need special types for units that are common, but generally use the same unit everywhere (i.e. Voltage, Current, Power).

@Ellerbach Yes, I was talking about terrestrial position, expressed as latitude, longitude and altitude. This should be a separate object, as it prevents lots of errors with i.e. exchange of latitude and longitude (seen that, been there...). Also, this cannot be expressed as a normal 3d-Vector, since the operations allowed on it are very different.

@Ellerbach
Copy link
Member

unless everyone is happy with Position, Position2, Position3, Velocity, Velocity2, Velocity3, Acceleration, Acceleration2, Acceleration3

I understand the point, so maybe yes, not in priority 1 and I'm happy with Position, Position 2, Position 3, etc. Again, agree we should wait then for this one.
Also, we may have to implement Kalman filters and other filters for those elements. They are must used in any robotic project. They are used to integrate and treat the accelerometer, compass and magnetometer.

this cannot be expressed as a normal 3d-Vector, since the operations allowed on it are very different

OK, got the point. So then it definitely makes sense to create a specific class with all the needed operations in it.

@pgrawehr
Copy link
Contributor

So then it definitely makes sense to create a specific class with all the needed operations in it.

These operations (i.e. greater circle distance between two points) are complex mathematical operations. There are algorithms (and also implementations) for that, but we'll have to check whether there's one with a compatible license. But that's an implementation detail, not exactly relevant for what we want.

@pgrawehr
Copy link
Contributor

pgrawehr commented Dec 8, 2019

I think we should also have types (or at least an enum) for one-dimensional values, such as Temperature (exists already), Voltage, Power, etc. This will allow a common formatting method, that converts measurements to strings with units, including a readable scaling (i.e. to mA or kV). We could even provide some operations on these units (i.e. voltage times current equals power).

@MarkCiliaVincenti
Copy link
Contributor

I reiterate what I said elsewhere that Microsoft needs a new package just for units.

@krwq
Copy link
Member Author

krwq commented Apr 24, 2020

@JTrotta has suggested to use https://github.com/angularsen/UnitsNet instead of re-inventing the wheel. So far looking at that it seems to me we can re-use the library.

License seem to be compatible. What are your thoughts about it?

@MarkCiliaVincenti do you mean you prefer to re-implement? (assuming you read the thread above looking at timing)

@krwq
Copy link
Member Author

krwq commented Apr 24, 2020

I'm currently asking for a green light if Iot.Device.Bindings can depend on that. Ideally would be nice if we could get that library's namespaced evolve to System.Units but we should start with checking if it works for us first

@krwq
Copy link
Member Author

krwq commented Apr 24, 2020

Got a green light. Plan is following:

  • let's create a branch in dotnet/iot: feature-units and start integrating that library
  • as we go we should write down all problems with it if there are any
  • if we notice this is not working well for us we back off
  • if we are happy with the branch we go forward with it and merge it into master

@krwq
Copy link
Member Author

krwq commented Apr 24, 2020

Anyone would like to help with integrating this? cc: @pgrawehr @JTrotta @MarkCiliaVincenti

@MarkCiliaVincenti
Copy link
Contributor

I would like to help but I think this falls in the remit of someone who is knowledgeable on physics; I'm purely on software development.

@krwq
Copy link
Member Author

krwq commented Apr 24, 2020

@MarkCiliaVincenti first step is to simply convert what we already have to new library. next step is to integrate more

@JTrotta
Copy link
Contributor

JTrotta commented Apr 24, 2020

Anyone would like to help with integrating this? cc: @pgrawehr @JTrotta @MarkCiliaVincenti

If someone creates the template/repository, I can help.

@krwq
Copy link
Member Author

krwq commented Apr 24, 2020

@JTrotta already created a branch in this repo (feature-units). First step would be to remove our Units library and add package reference to Iot.Device.Bindings to UnitsNet and replace all failing places with new library and equivalent APIs

@JTrotta
Copy link
Contributor

JTrotta commented Apr 24, 2020

I see. So you do not want to create a new library, but integratate it into IOT, do you?
You want remove Iot.Units namespace and src under Device folder, and start to refer to the external to check functionalities or you want to integrate source code?

Sorry for questions, but I'd like to better understand your aim.

@MarkCiliaVincenti
Copy link
Contributor

I see. So you do not want to create a new library, but integratate it into IOT, do you?
You want remove Iot.Units namespace and src under Device folder, and start to refer to the external to check functionalities or you want to integrate source code?

Sorry for questions, but I'd like to better understand your aim.

Yeah, I didn't understand either.

@pgrawehr
Copy link
Contributor

See coment from krwq above ("got green light"...)

He has created a branch for integrating that library. The task for whoever picks this up are first:

  • Remove our own classes (Temperature and Pressure pressumably)
  • Add reference to the UnitsNet package
  • Try to build
  • If it works, submit a PR against the "feature-units" branch

Then we can try to make more use of that library and find whether it misses features we need or has other issues.

@krwq
Copy link
Member Author

krwq commented Apr 24, 2020

exactly as @pgrawehr has wrote, sorry for confusion 😄

@krwq
Copy link
Member Author

krwq commented Apr 25, 2020

I'll start with initial PR

@krwq
Copy link
Member Author

krwq commented Apr 25, 2020

Let me know what you think. So far I like the result of this experiment and I think we should keep going with integrating the units. @joperezr?

@krwq
Copy link
Member Author

krwq commented Apr 25, 2020

if we decide we like it I think we should go with some multidimensional units (so perhaps convert IMUs) and see how that code looks like. After that if we decide we want this permanently we should merge that into master and work on master directly

@krwq
Copy link
Member Author

krwq commented Apr 25, 2020

Ok, I have just tried converting BNO055 to use MagneticField but couldn't see any way of creating a 3 dimensional vector out of that with UnitsNet. I've created an issue there to see what they recommend: angularsen/UnitsNet#780

@krwq
Copy link
Member Author

krwq commented Apr 27, 2020

anyone interested in continuing the effort? the multi-dimensional units seem to be out of scope for now (.NET currently doesn't allow to put restrictions on generic types such that you could restrict type to numbers only and use operators which seem to be the reason why UnitsNet haven't added any multidimensional units - they have some workaround in progress and I also asked our PM if this is something they plan for the future).

we should focus on finding which units can be integrated into our repo and do some of that integration.

@pgrawehr
Copy link
Contributor

You mean that we should find out where we would rather use units instead of plain doubles?
I guess that needs going trough all the bindings and check that. My "big" IOT project is taking some time now as well, but I'll try to find some time for a few known bindings later. And I'm still also working on the Nmea stuff, which will use many units.

How do we avoid introducing all kinds of breaking changes? Duplicating properties is probably not a good idea (i.e. existing "Voltage" property and now "VoltageAsRealVoltage").

@krwq
Copy link
Member Author

krwq commented Apr 27, 2020

Yes for the question. For the breaking changes: I personally feel flexible here but my opinion is that for Iot.Device.Bindings we should go with breaking changes although reducing them to as little as possible (if any property had unit in the name and returns double then it should still be there but if it is called say Voltage which would clash with name we'd like to use now I think we should change the type in those cases). People have a choice of still using the older versions of the package but in order to move forward they will need to do some minor changes... I think we'd do the same if we've added more units to our Units and I think this is something we should do.

@joperezr
Copy link
Member

How do we avoid introducing all kinds of breaking changes? Duplicating properties is probably not a good idea (i.e. existing "Voltage" property and now "VoltageAsRealVoltage").

Regarding that it is worth noting that Iot.Device.Bindings package doesn't have the same level of guidelines that System.Device.Gpio does, and we do allow making breaking changes as long as they are valuable and it is easy for consumers to adapt to the new breaking changes which I believe we are fine on both of those items with this.

EDIT: looks like @krwq beat me for a few seconds haha, but answer is pretty much the same. 😄

@pgrawehr
Copy link
Contributor

@krwq: Can you please rebase the branch? The removed platform dependency will remove issues with VS playing havoc on reference assemblies, which is ugly to work with if one needs to go trough a lot of bindings.

@joperezr
Copy link
Member

Can you please rebase the branch? The removed platform dependency will remove issues with VS playing havoc on reference assemblies, which is ugly to work with if one needs to go trough a lot of bindings.

Done. Make sure whoever is going to work on this to do a fetch again in case they had already pulled down the branch as when you do a rebase the history is changed so local branches will be broken and will need a re-fetch.

@krwq
Copy link
Member Author

krwq commented May 19, 2020

feature-units branch is merged now which means we are officially using UnitsNet now. I'll close this issue and replace it with issue about adopting this library better

@krwq krwq closed this as completed May 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Oct 14, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-suggestion Early API idea and discussion, it is NOT ready for implementation
Projects
None yet
Development

No branches or pull requests

7 participants