Skip to content
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

Telemetry modules #1835

Merged
merged 12 commits into from Oct 7, 2016
Merged

Telemetry modules #1835

merged 12 commits into from Oct 7, 2016

Conversation

gautierhattenberger
Copy link
Member

@gautierhattenberger gautierhattenberger commented Aug 6, 2016

Convert telemetry subsystems to modules.

  • needs hardware testing
  • hard fault recovery need telemetry modules init

The second point is not done yet since modules init are part of the global AP thread. Should I just call all modules init (and downlink init) or change the modules generator to be able to group functions (will be useful when converting FBW subsystems to modules) ?

only wrappers (replace makefiles) at this stage
hard-fault recovery is an issue since downlink should be initialized
even if AP is not
PPRZLINK should be used now
in particular drop audio_telemetry code
@podhrmic
Copy link
Member

podhrmic commented Aug 6, 2016

@gautierhattenberger IMHO it would make sense if each of the modules had their own thread with a separate recovery mechanism (since eventually everything will be a module).

Should we also mutex critical resources in the telemetry modules? Because I could imagine that now you can easily have multiple telemetries running at the same time - so access to lets say state variables etc. should be protected, right?

@gautierhattenberger
Copy link
Member Author

Not sure it is really needed to have each module in a thread, but we can definitely group them (already have some sort of tasks: navigation, control, safety, payload, ...).
Regarding the access of shared resources, we need to discuss that a little bit (and probably not in this thread). I already asked if state interface for instance should be protected in a transparent way like what you did in your branch, or if anyone using it should request for a mutex to use it (second option being more efficient but ask for a lot of work). At least the current implementation allows multiple threads to send messages other the telemetry system (it already the case with AP and FBW).

My real concern is more about the practical implementation and naming:

  • should we have all the functions (init, periodic, event) of a module in the same "thread" or should we specify this for all functions independently.
  • mix the two options: a global value can be defined, overwritten by local ones if needed
  • what is the best name: process (not nice, but we already use this key word in other config files), thread (not really better I think, can be misleading if bare metal version is used), task (I would choose this one), group (not really nice and too generic). Any better idea ?

@gautierhattenberger
Copy link
Member Author

see #243

@podhrmic
Copy link
Member

podhrmic commented Aug 8, 2016

Just quickly about the mutexes - the current solution (protect the state interface) is IMHO the way to go (at least for now).

About your questions:

should we have all the functions (init, periodic, event) of a module in the same "thread" or should we specify this for all functions independently.

I would put it all in the same "thread" - like you start a thread, and it initializes what it needs. Should be easier to use and more error-proof.

mix the two options: a global value can be defined, overwritten by local ones if needed

Can you give an example? I am not sure if I fully follow.

what is the best name: process (not nice, but we already use this key word in other config files), thread (not really better I think, can be misleading if bare metal version is used), task (I would choose this one), group (not really nice and too generic). Any better idea ?

Task +1

@flixr
Copy link
Member

flixr commented Aug 8, 2016

For most modules it probably doesn't make sense to give them their own thread...

Task is probably an OK name

The previous names can be used to call functions from all tasks.
This is a simple solution to fix hard-fault recovery (datalink modules
init).
It can be extended to other modules but probably need some more work.
@gautierhattenberger
Copy link
Member Author

Tested and working on:

  • sim and nps
  • pprz and xbee on STM32F4
  • ardrone2

anyone for testing superbit and bluegiga ?

@podhrmic
Copy link
Member

I can test Lisa F1 today (transparent), although I don't expect any problems here.

@flixr
Copy link
Member

flixr commented Aug 19, 2016

Tested and working:

  • transparent and transparent_usb on LisaMX (STM32F4)
  • transparent on LisaM 1.0 (STM32F1) (usb_serial does not work on v1.0, but should work on later versions)

@flixr
Copy link
Member

flixr commented Aug 19, 2016

@fvantienen @kirkscheper could you test superbitrf and bluegiga?

@kirkscheper
Copy link
Member

I am on vacation :) also I've been working on an old master on a big
project, will update and test when I am back.

Kirk

On 19 Aug 2016 4:09 p.m., "Felix Ruess" notifications@github.com wrote:

@fvantienen https://github.com/fvantienen @kirkscheper
https://github.com/kirkscheper could you test superbitrf and bluegiga?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1835 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACtEQ_EW9x-8IYMl0nLaeqFBcwHD2zzIks5qhf9sgaJpZM4JeRQv
.

@flixr
Copy link
Member

flixr commented Sep 17, 2016

@kirkscheper have you had a chance to test bluegiga yet?

Copy link
Member

@flixr flixr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@kirkscheper
Copy link
Member

No, can test it on Monday though, that ks for the reminder.

Kirk

On 17 Sep 2016 9:01 p.m., "Felix Ruess" notifications@github.com wrote:

@kirkscheper https://github.com/kirkscheper have you had a chance to
test bluegiga yet?


You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#1835 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/ACtEQxXhQKBKjhMR9jEm9W0WkD_Ja98Mks5qrDkPgaJpZM4JeRQv
.

@flixr flixr mentioned this pull request Sep 20, 2016
12 tasks
@kirkscheper
Copy link
Member

Bluegiga doesn't work with this branch but actually it doesn't currently work with master either. I have been using a branch based on an older master so haven't noticed this. I don't receive any downlink messages... Have there been any changes to SPI lately?

@gautierhattenberger
Copy link
Member Author

I don't think so. Can you give us the reference of the working version ?

@kirkscheper
Copy link
Member

I am trying some older versions so see where it breaks. get back to you soon

@kirkscheper
Copy link
Member

First break happens at commit Coverity fixes (#1838) b68d166. An error in that commit prevents correct messages from being interpreted by the GCS. I still receive some downlink data though.

When the imu refactor is commited however with [imu] convert imu subsystems to modules (#1788) 17d3277 the downlink is totally broken.

@gautierhattenberger
Copy link
Member Author

Regarding the fix b68d166 can you try to revert the line 238 in bluegiga.c (although it should be more dangerous).
For the imu modules, it is really not related to datalink stuff, that's really strange.

@kirkscheper
Copy link
Member

The imu update does play around with spi and i2c defaults though. I am guessing it has something to do with that.

@kirkscheper
Copy link
Member

One difference is that the new lisa s imu defines ASPRIN_2_I2C and USE_I2C while the old one didn't. The ap_srcs.list doesn't define those two at all. Any chance that might have any effect?

@kirkscheper
Copy link
Member

ok, on a hunch I removed those definitions from master and it works again...

@kirkscheper
Copy link
Member

Same fix fixes this PR so it seems like this refactor also works for Bluegiga.

@gautierhattenberger
Copy link
Member Author

can you try to add case="upper|lower" to the lines configuring the SPI and I2C dev in the file imu_lisa_s_v1.0.xml ?

@kirkscheper
Copy link
Member

Nope, doesn't work.

@gautierhattenberger
Copy link
Member Author

do you have your patch in a branch somewhere ?

@kirkscheper
Copy link
Member

https://github.com/kirkscheper/paparazzi/tree/bluegiga_fix

Its is strange the i2c definition was not in the previous xml for aspirin or lisa/s

gautierhattenberger added a commit that referenced this pull request Sep 20, 2016
this was produce a strange side effect on bluegiga module (#1835)
@gautierhattenberger
Copy link
Member Author

@kirkscheper can you test the patch of #1858 ?

@flixr
Copy link
Member

flixr commented Sep 26, 2016

so I guess this is good to merge now?

@gautierhattenberger
Copy link
Member Author

It seems to work. I have only one concern: it is changing a little bit the module generator to have functions split in different "task". This part have not been tested a lot so far. Even if it should be fine, I initially planed to keep this for after the release. If you think it is fine, feel free to merge it.

merge master and update after changes from #1879
@flixr
Copy link
Member

flixr commented Oct 2, 2016

Maybe needs a quick re-test after latest changes?

@gautierhattenberger
Copy link
Member Author

re-tested with pprz and xbee over model, and pprz over udp (and it is still working)

@gautierhattenberger
Copy link
Member Author

can I merge this one ?

@flixr
Copy link
Member

flixr commented Oct 7, 2016

LGTM

@dewagter
Copy link
Member

dewagter commented Oct 7, 2016

Nice work, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants