QUB-ASL / bzzz

Quadcopter with ESP32 and RaspberryPi
MIT License
7 stars 0 forks source link

Python interface for barometer #163

Closed alphaville closed 4 months ago

alphaville commented 7 months ago

Describe the solution you'd like

Some updates may be necessary in PressureSensor. The barometer can provide useful measurements that along with ToF measurements can improve the estimation of altitude:

Documentation

All methods - private and public - should be documented. Currently there is no documentation.

 No setters

I suppose setters should not be available:

@altitude.setter
    def altitude(self, val):
        self._current_altitude = val

The caller should not be allowed to set a pressure or altitude value.

Naming conventions

We should update the names of functions and variables to be compatible with PEP8.

Private methods

Private methods should start with a double underscore (not single)

Refactoring

It's not clear to me what _pressure_readings_list does and why it is (not) initialised with None here and then initialised as a list here.

In any case, we should probably have an implementation akin to the one for the anemometer.

Testing

We need to test it again on the new PCB

Yuanbwcx commented 7 months ago
@altitude.setter
    def altitude(self, val):
        self._current_altitude = val

Although a setter method exists, I think it does not actually perform any substantial setting operations. It raises an exception telling the caller that the property cannot be set directly.

alphaville commented 7 months ago

@Yuanbwcx exacty - that's what I said in the issue description. I don't think we need a setter.

Yuanbwcx commented 7 months ago

@alphaville Yes, that make sense.

Yuanbwcx commented 7 months ago

I suppose _pressure_readings_list is to create a list to store pressure data, should we import Data Logger like the one for the anemometer?

alphaville commented 7 months ago

I suppose _pressure_readings_list is to create a list to store pressure data,

I would ignore the majority of the code that is written there. It is not very well designed.

should we import Data Logger like the one for the anemometer?

Yes, import both data_logger and filters

Runway27 commented 4 months ago

Can I close this issue as well?

alphaville commented 4 months ago

@Runway27, yes, I'm closing this now since you merged the corresponding PR.