struct TransceiverState

[AsbjornPettersen]

The struct TransceiverState contains a lot of things: User configuration, board configuration,,, !

I'm planning to move all "agc_" variables into one single "agc" structure.
Then we don't need the "agc_" nameing anymore.

Example:
struct TransceiverAGCState
{
uchar mode;
uchar custom_decay;
uint8_t wdsp;
uint8_t wdsp_mode;
uint8_t wdsp_slope;
uint8_t wdsp_hang_enable;
int wdsp_thresh;
int wdsp_hang_thresh;
int wdsp_hang_time;
uint8_t wdsp_action;
uint8_t wdsp_switch_mode;
uint8_t wdsp_hang_action;
int wdsp_tau_decay[6];
int wdsp_tau_hang_decay;
};

typedef struct TransceiverState
{
...
// AGC mode
struct TransceiverAGCState agc;
....
};

Then i have to rename all ts.agc_mode to ts.agc.mode ,,,,
Low risk and no real code changes.
Ok ?

[DD4WH]

No.
Please stick to our rule:
Do not change the code unless you can offer a performance increase.
I will not accept changes in the structure of the code, that are made just because of the structure.
I am not a professional programmer and I will be unable to further contribute to the code if the structure of the code changes dynamically just because programmers have fun in changing the structure without contributing to the performance of the mcHF.
Sorry, Asbjorn, I do not mean to be rude to you, it is just that I would be willing to continuously contribute to the code in the future with DSP input. But I will not be doing this if the code structure changes a lot just because of the idea of an elegant code.

All the best, 73 de Frank DD4WH

[db4ple]

Hi,
well, I am basically in favor of this change. It does not change the code really, it provides a better structure AND it will increase performance (a little) if we move the structure of the TransceiverState structure. How come: the variable TransceiverState ts is marked as "volatile" which generates by the nature of the keyword code which ALWAYS reads the data from the memory and every write directly goes to the main memory. No real optimization can take place because of the "volatile" ( aka __IO ). The use of volatile for ts is largely unnecessary but in some places required (for information which is shared between normal and interrupt driver code in loops in the normal code waiting for change by the interrupt driver code . But for the correct function of AGC this is not necessary.

However, we could also postpone this for a time when we refactor the audio driver which is now a 4500+ line monster.

[DD4WH]

Hi Danilo,
we have been discussing these kinds of changes several times. If there is no really urgent need to do this kind of changes, please avoid them.
The reason is that for non-professional programmers like me the mcHF code is already a major challenge.
If professional programmers now start to refactor the code, people like me (amateur programmers) will have no more chance to contribute,because the code will become unreadable (at least for me: I know what I am talking about).
Sure, you can do that in order to get out the last bit of performance and create elegant code. But in that case, if the audio_driver is going to be further refactored (you already broke the code into small pieces which I am unable to put together in my head ;-)), I will be unable to follow those changes any more.
So from my point of view: leave the code as it is, if it is not really necessary to change it.
If you want to change it, because it is really necessary: go ahead! But do not expect me to be able to follow those changes . . .
73 de Frank DD4WH

[AsbjornPettersen]

This is not a FUN job, it's a dirty job. Only risk and complaints, but the gain comes later.
The "only fix bug" strategy will result in complex and buggy/unreadable code.
This "only fix bug" strategy is VERY good for a stable branch, and disaster for a development branch.
A stable branch could be 2.2.x and development 2.3.x, and when development are finished we call it 2.4.0 (another stable)

example:
2.2.0 Stable release where only bug fixes are allowed
2.3.0 Unstable release where all new development are done.
2.4.0 Stable release where only bug fixes are allowed (old 2.3.x branch)
2.5.0 Unstable release where all new development are done.
....

I assume that "ts.agc.mode" are readable as "ts.agc_mode".
The "agc" variables are all over the place in struct TransceiverState !

[df8oe]

We do have a fundamental (mostly philosophical) problem here. There are two opinions which cannot come together:

1. Some want to bring well structure to code and do all the things good readable code can be obtained

2. Others build a picture in their head of exactly the code which exist and if this picture does not show reality of code (because it is reworked) they loose ability of understanding it.

I am not a professional programmer at all. And I started programming by doing very big monolithic blocks, creating global varaibles and so on (all the things one might not do). If I saw other, good-structured code I was not able to understand it because I missed to learn otherwise than in monolithic blocks. In the meantime I opened my mind and now it is much easier to understand code that is not in a monolithic block but in many well-named sub functions. So if I see a "monster" of 4.500 lines now I think only the ones who have grown up together with this monster can understand it. Everyone who see it first time cannot understand what is going on.

So I agree that refactoring of code has very high priority and is a MUST. I guess there are some bugs sleeping due to this "bad programming behaviour" and will some up sometimes later and will not be recovered simply. The first very big step mcHF firmware has done was the time Danilo refactored Clints code and recovered some out-of-bounds, undefined variable states and so on. I think we must not discuss if refactoring is a proof or not.

It is not a solution to allow specific changes only in specific branches. This only will win time for the ones who are not able to understand refactored code. The deep problem only will come up some commits or merges later.

The goal has to be to take care that the ones who are familar only with the existing code and are not able by themself to go 10 steps awy and see the new structure from bigger distance. I have thought about that problem many times and I have not found a solution yet. Frank is a very good DSP programmer and if he does not continue working on the code we do not have anyone who is able to do that work.

Actually we do have block diagrams which are automated produced with every new build which shows changed structure. And breaking monolithic parts in many small but logical parts are better readable to persons who see the code the first time but maybe unreadable to someone who relies upon that these monolithic blocks never will change....

What can be done to satisfy BOTH parts, to hold our team together? What can be done to bring understanding of "well structured and refactored code" to all of us?

[AsbjornPettersen]

Well i can stay away from the DSP area, since there a lot of other things to do :)

Here another example for UI part:

uchar waterfall_color_scheme; // stores waterfall color scheme
uchar waterfall_vert_step_size; // vertical step size in waterfall mode
int32_t waterfall_offset; // offset for waterfall display
ulong waterfall_contrast; // contrast setting for waterfall display
...
uchar waterfall_nosig_adjust; // Adjustment for no signal adjustment conditions for waterfall
..
??
uchar spectrum_size; // size of waterfall display (and other parameters) - size setting is in lower ..

struct WaterfallConfiguration
{
uchar color_scheme; // stores waterfall color scheme
uchar vert_step_size; // vertical step size in waterfall mode
int32_t offset; // offset for waterfall display
ulong contrast; // contrast setting for waterfall display
uchar nosig_adjust; // Adjustment for no signal adjustment conditions for waterfall
/* ???? */
uchar spectrum_size; // size of waterfall display (and other parameters) - size setting is in lower ..
};
..
struct WaterfallConfiguration waterfall;
..

[f4fhh]

Hi all

Hobbyist programmer here. I discovered the internals of the mchf code some days ago just after completing the HW build. My primary goal was to add some missing features like a memory management system. I must say that I had a hard time to figure out the structure of the code and I think that a better modularization/factorization would help me a lot. However, I also agree that we must preserve the audio code of any disruptive changes because it is at the core of the project and the work of Franck is totally awesome.

Concerning the TransceiverState struct, I think that its factorization is unavoidable if we want to implement effectivelly some features like memories that store frequencies, modes, agc settings, etc.

73 de Nicolas F4FHH

[df8oe]

Hi Nicolas,

fully agree. More structured and modularized code would help much. If you take a look at the "old" sources (versions beginnig with 219....) you will see the improvements that already have been implemented.

And regarding "memory places":
Please let us talk about that before starting anything. We are planing not only the addition of memory places but additional informations that are stored in each "place" (included the band-memory-places like we are already working with). At the recent stage we have not done thoughts about what can/must be stored and how structure can be. And there is much to think about!!

73 de Andreas, DF8OE

[DD4WH]

Hi all,
@asbjorn: no need to stay away from the DSP coding! You are very welcome to contribute also to that part of the code! There is no area in the code that is "reserved" to anybody (from my point of view). But it would be nice to discuss about potential plans and changes before doing a major rework of the DSP code ;-).
@andreas: yes, you are right that it is a different philosophy, but that philosophy difference produces real practical problems, and not only philosophical problems (at least for me ;-)).
@Danilo: yes, I also agree that refactoring is a good thing to improve functioning and error finding etc. But the difficulty for me is how to keep track of the structure of the audio path and remain able to contribute. That is sometimes very hard for me if there are major changes in the code and the linear structure of the audio path is broken up into many small pieces scattered in the code.
The idea by Andreas & Nicolas to first develop a structure together, that we all agree upon and then in a second step to alter the code would be a good thing, as far as the parts of the code are concerned that I normally deal with (audio_driver, ui_spectrum).
One of the longterm projects could be to develop a plan how to totally rebuild the audio_driver in the frequency domain (FFT - filtering etc. - inverse FFT). That would be a major step forward, also for the new processor STM32F7.
And I do not think the audio_driver is a monster. It is comparatively well-behaved ;-).
73 de Frank

[yo2ldk]

Working at STM32f7 is a great news, and I thinking to ask, but I know you are very busy.
This can made a good pair with WM8860 audio on 192kHz, do you think can be made it?
My thought is on a different modular transceiver, not only for portable, maybe with AV or VGA output also
Anyway, Congratulations for all your hard job, 73 !

[AsbjornPettersen]

Lately I’m been looking at the bootloader code, and tried to remove all unused c files.
But when I get into the header files, there are problems.

The bootloader build are depended on struct TransceiverState, and audio_filter.h,,,
No need for them at all in the bootloader (user config, audio filtering,,) !?
Every change in audio_filter.h,, will require a rebuild of the bootloader, and hopefully without any side effects.

Too fix the problem the global TranceiverState struct must be decoupled from some functions by adding parameters.
The bootloader code in main.c must be moved to bootloader_main.c, and the find the header files that are really needed.

[db4ple]

Asbjorn,
the bootloader build does not depend on the struct TransceiverState ts, it just includes the header file mchf_board.h, which happens to provide a number of definitions of types and constants shared between the bootloader build and the firmware build. So some changes to this file may change bootloader binary, others don't. I am not saying this could not be done in a better way but bootloader.map shows clearly that the variable ts (which is the only variable of type TransceiverState) is not being included in the bootloader build. So the question is: Which functions you are refering to?

Danilo

[AsbjornPettersen]

The compile of the bootloader require this struct.
I know the linker doing a good job to remove the compiled structure.
Found out today that the mchf_rtc.c can be removed from the bootloader.mak !

Functions that compiles and later removed by the linker:
bool MchfRtc_enabled()
{
..
ts.vbat_present = true;
..
}

inline bool is_ssb_tx_filter_enabled();
inline bool is_splitmode();
void mchf_board_init(void);
void MchfBoard_HandlePowerDown() ;
void mchf_board_post_init(void);
void mchf_board_detect_ramsize();
uint16_t ConfigStorage_ReadVariable(uint16_t addr, uint16_t *value)
uint16_t ConfigStorage_WriteVariable(uint16_t addr, uint16_t value)
void ConfigStorage_Init();
void ConfigStorage_CopyFlash2RAMCache();
void ConfigStorage_CopySerial2RAMCache();
uint16_t ConfigStorage_CopyRAMCache2Serial();
static void ConfigStorage_CheckSameContentSerialAndFlash(void);

[db4ple]

Asbjorn,

in the db4ple/f7 branch some corrections have been made to the bootloader and none of these functions (or files which they are contained in) are any longer compiled for the bootloader. If you can't wait until this branch is merged, you have to remove the call to mchf_board_touchscreen_init() from bootloader_main.c and remove the files

misc/v_eprom/eeprom.c
misc/config_storage.c
misc/profiling.c
misc/serial_eeprom.c
hardware/mchf_board.c
hardware/mchf_rtc.c \

from bootloader.mak (and submit a pull request if you want).