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

Add Documentation for PruManager #4

Open
wants to merge 1 commit into
base: BBAI-demo
Choose a base branch
from
Open

Conversation

DhruvaG2000
Copy link
Collaborator

Add Doxygen supported Documentation for PruManager Implementation

@@ -17,6 +17,12 @@
#include <vector>
#include "Mmap.h"

/**
* \brief Support for interaction with PRU via
* (rproc+mmap) and/or (uio+libprussdrv)
Copy link
Owner

Choose a reason for hiding this comment

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

This is the description of the base class, so it should not contain any references to the children classes, as there may be more added in the future that you currently don't know about.

public:
/**
Copy link
Owner

Choose a reason for hiding this comment

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

I would expect all the documentation for the methods to be in the base class. If any child class has specific exceptions, those can be documented in the children class. (e.g.: PruManagerUio does not support pruNum >= 2, that should be documented in the child class as an exception).

This is assuming Doxygen presents it all in a nice format.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay then I am leaving the base class' pruNum parameter empty as it is child class dependent.

Copy link
Owner

Choose a reason for hiding this comment

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

I'd rather have it in the base class instead, so that children classes know what the specs are. Then if they have exceptions, they should document it appropriately.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, added in 7c0e7b9

PruManagerRprocMmap(unsigned int pruNum, int v);
/**
* performs echo stop > state
Copy link
Owner

Choose a reason for hiding this comment

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

performs echo stop > state is an implementation detail. You can leave that in the .cpp file, but there is no need for the user to know this. With regard to what mentioned above of putting the description in the base class, the stop() method does not need to be documented in this child class, as the only difference is in the implementation details.

void stop();
/**
* checks whether to use McASP IRQ or non McASP IRQ PRU code and then calls
Copy link
Owner

Choose a reason for hiding this comment

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

What about

Loads the firmware and starts the PRU. It uses the one of the firmwares built with Bela.
@param useMcaspIrq if `true`, uses `pru_rtaudio_irq.p` , if `false` uses `pru_rtaudio.p`

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

int start(bool useMcaspIrq);
/**
* Load the firmware and start the PRU
* @param path path to the appropriate PRU firmware `.out` file
Copy link
Owner

Choose a reason for hiding this comment

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

when moving this description to the base class, keep it generic "path to a PRU firmware file that is suitable to be loaded by the child class's underlying driver".

Copy link
Owner

Choose a reason for hiding this comment

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

then in the child class you can specify what the appropriate format is (i.e.: a binary of the firmware for Uio and an ELF .out suitable for rproc for the other one)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay sure, done

int start(const std::string& path);
/**
* accesses the DATA RAM0 and DATA RAM1<br>
Copy link
Owner

Choose a reason for hiding this comment

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

"Obtain a pointer to the PRU's own DATA RAM"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

void* getOwnMemory();
/**
* accesses the general purpose memory RAM (signified RAM2) shared between PRU0 and PRU1
Copy link
Owner

Choose a reason for hiding this comment

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

"Obtain a pointer to the PRUSS shared RAM"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

Copy link
Collaborator Author

@DhruvaG2000 DhruvaG2000 left a comment

Choose a reason for hiding this comment

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

tried to make all changes suggested so far

public:
/**
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay then I am leaving the base class' pruNum parameter empty as it is child class dependent.

void stop();
/**
* checks whether to use McASP IRQ or non McASP IRQ PRU code and then calls
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

int start(bool useMcaspIrq);
/**
* Load the firmware and start the PRU
* @param path path to the appropriate PRU firmware `.out` file
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

okay sure, done

int start(const std::string& path);
/**
* accesses the DATA RAM0 and DATA RAM1<br>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

void* getOwnMemory();
/**
* accesses the general purpose memory RAM (signified RAM2) shared between PRU0 and PRU1
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

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

Successfully merging this pull request may close these issues.

2 participants